Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove postinstall step #28

Merged
merged 1 commit into from
Nov 7, 2017
Merged

Remove postinstall step #28

merged 1 commit into from
Nov 7, 2017

Conversation

demurgos
Copy link
Contributor

Use a prepare hook instead of a postinstall hook to generate
the highlight_alias.json file. This means that the file is generated
during the publication of package to npm or when manually running
npm install in the project directory.
The previous behavior was to generate this step on the consumer
side: this was the cause of numerous hard-to-reproduce issues due
to the variety of environments.

Closes #20
Closes #23
Closes #24
Related to hexojs/hexo-cli#23
Related to hexojs/hexo#2095

@coveralls
Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage remained the same at 96.21% when pulling 00e2f0f on demurgos:issue-24 into f8e237a on hexojs:master.

@demurgos demurgos force-pushed the issue-24 branch 3 times, most recently from 2ac21d3 to d546898 Compare October 26, 2017 09:01
@coveralls
Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage remained the same at 96.21% when pulling d546898 on demurgos:issue-24 into f8e237a on hexojs:master.

@NoahDragon
Copy link
Member

Code LGTM. Could you please remove the package lock json file?

Use a `prepare` hook instead of a `postinstall` hook to generate
the `highlight_alias.json` file. This means that the file is generated
during the publication of package to npm or when manually running
`npm install` in the project directory.
The previous behavior was to generate this step on the consumer
side: this was the cause of numerous hard-to-reproduce issues due
to the variety of environments.

Closes hexojs#20
Closes hexojs#23
Closes hexojs#24
@demurgos
Copy link
Contributor Author

demurgos commented Oct 27, 2017

@NoahDragon
Sure, I removed it. Still, it's good practice to to include it so it is possible to get a reproducible build in case of issue. It will only apply for developers of the library (and won't interfere with other modules when installed as a dependency). More information.

This file is intended to be committed into source repositories

@coveralls
Copy link

coveralls commented Oct 27, 2017

Coverage Status

Coverage remained the same at 96.21% when pulling 512fc5c on demurgos:issue-24 into f8e237a on hexojs:master.

Copy link
Member

@NoahDragon NoahDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@demurgos
Copy link
Contributor Author

demurgos commented Nov 3, 2017

Hi,
What is the process to merge a PR and push it to npm once it was approved? The issue solved by this PR currently breaks any attempt to npm install this package on my system (and requires me to use yarn install instead).

@NoahDragon NoahDragon merged commit ae4bd8a into hexojs:master Nov 7, 2017
@NoahDragon
Copy link
Member

@demurgos will publish soon.

@demurgos
Copy link
Contributor Author

demurgos commented Nov 7, 2017

Thanks 👍

@NoahDragon
Copy link
Member

@demurgos Published.

@demurgos
Copy link
Contributor Author

demurgos commented Nov 7, 2017

I confirm that it fixed the issue on my system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants