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

Tweaked install/publish related scripts - enabled installing as dep d… #1670

Merged
merged 1 commit into from
Oct 8, 2017

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Oct 8, 2017

…irectly from git

Fixed issue mentioned in rollup/rollup-plugin-babel#100 (comment)

This enabled installing rollup directly from github, convenient for testing PRs etc in other projects.

cc @lukastaegert

package.json Outdated
@@ -19,7 +20,9 @@
"ci": "npm run test-coverage && codecov < coverage/coverage-remapped.lcov",
"build": "git rev-parse HEAD > .commithash && rollup -c && chmod a+x bin/rollup",
"watch": "rollup -cw",
"prepublish": "npm run lint && npm test && npm run test:leak",
"prepublish": "test $(npm -v | tr . '\\n' | head -n 1) -ge '4' || exit 1",
Copy link
Member Author

Choose a reason for hiding this comment

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

because prepare is npm@4 thing, im guarding here against publishing with lower versions which could potentially lead to broken published version, as fresh build wouldnt get executed

@Andarist
Copy link
Member Author

Andarist commented Oct 8, 2017

The PR has failed on the CI on node 6 and node 4, which come respectively with npm 3 and 2. Aint sure what you'd like me to do here - i.e. babel was just installing npm 4 globally, to prevent similar issues - https://github.com/babel/babel/blob/e522dd8a3c200899fd37c5570049e84ef9538c80/.travis.yml#L23-L25

@lukastaegert
Copy link
Member

Since it is only on CI and should not affect the resulting bundle which is still tested against the same node versions I do not see an issue with Babel’s approach.

@Andarist Andarist force-pushed the enhancement/as-git-dep branch 3 times, most recently from ddc792f to f1de371 Compare October 8, 2017 17:33
@Andarist
Copy link
Member Author

Andarist commented Oct 8, 2017

@lukastaegert Done.

@lukastaegert
Copy link
Member

Installing from your branch works like a charm, I wan't aware prepare was so powerful. It seems like it downloads the dev dependencies behind the scenes, uses them to create the artefact but then only puts the artefact into node_modules–just magic.

The only thing that keeps me from merging is that (apparently the Windows environment of) appveyor does not like the version test, apparently there is too much bash magic involved.

Isn't this also what the engines: field in package.json is meant for? This will not prevent publishing with the wrong npm version but at least show a warning. But a strict safeguard would certainly be better.

@lukastaegert
Copy link
Member

Oh I see you had already removed the test.

@lukastaegert
Copy link
Member

Thinking about it, installing from github urls should be an exception anyway and in case anyone has troubles we could just tell them to update npm. I will merge this now.

@lukastaegert lukastaegert merged commit 63d9ef6 into rollup:master Oct 8, 2017
@Andarist Andarist deleted the enhancement/as-git-dep branch October 8, 2017 20:22
@Andarist
Copy link
Member Author

Andarist commented Oct 8, 2017

Installing from your branch works like a charm, I wan't aware prepare was so powerful. It seems like it downloads the dev dependencies behind the scenes, uses them to create the artefact but then only puts the artefact into node_modules–just magic.

Since v5 - "Git dependencies with prepare scripts will have their devDependencies installed, and npm install run in their directory before being packed."

Isn't this also what the engines: field in package.json is meant for? This will not prevent publishing with the wrong npm version but at least show a warning. But a strict safeguard would certainly be better.

Would be great to use it, but it also warns for consumers of the package when the package is installed and we do not want that behaviour. I've creates issue about this npm/npm#18756 as it annoys me for quite some time already.

The only thing that keeps me from merging is that (apparently the Windows environment of) appveyor does not like the version test, apparently there is too much bash magic involved.
Oh I see you had already removed the test.

Yeah, I've removed this test because of that - put it there in the first place to avoid wrongful publication in case of somebody on rollup's team uses older npm. This has happened to me already some itme ago, a team member published a package without output files :D

I wanted to create a package to solve this slight problem - https://github.com/Andarist/package-lifecycle . But ditched it for now, as gain is minimal and I do not know perfect solution - was planning to use npm .hooks feature, but it has its downsides.

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.

2 participants