Fix npm package #532

autra · 2017-10-13T14:37:17Z

Description

These are a collection of small fixes in our packaging system.

tbroyer

This is a somewhat breaking change, so should be highlighted in the release notes for the next release (for those who wouldn't use three, or meshline, in their apps)

autra · 2017-10-13T17:16:28Z

This is a somewhat breaking change, so should be highlighted in the release notes for the next release (for those who wouldn't use three, or meshline, in their apps)

Normally, they shouldn't have to do anything (three and three.meshline are still in dependencies)

tbroyer · 2017-10-13T17:59:26Z

(three and three.meshline are still in dependencies)

Oops, missed it.
Shouldn't they be removed then though? What's the goal of adding them as peer dependencies but keeping them as dependencies? Making sure the correct version is being used? (given that almost all update of three has breaking changes, I'm not sure the configuration is correct there then)

autra · 2017-10-13T19:47:27Z

My understanding (to be taken with a grain of salt) is that if a project A declares B as peerDependencies, B used to be installed automatically when you do npm install in A folder. Apparently, as of npm 5, that has changed (according to my tests) and it no longer installs anything on its own. I think the rationale is that you might want to declare you're compatible with a specifi version of a package, without actually explicitly depends on this package.

Making sure the correct version is being used?

On downstream projects yes. It only displays warnings when there's a mismatch though (they should really be errors with a -f flag imho, but nobody cares about my npm opinions it seems!).

Long story short, as we still need three to be in itowns' node_modules, we need it in dependencies as well.

Onto the version specifier question: in semver, all 0.x versions are considered major. Therefore, while ^1.0 would include every 1.x versions, ^0.77 would only include 0.77.x versions. From https://docs.npmjs.com/misc/semver on caret range:

Allows changes that do not modify the left-most non-zero digit in the [major, minor, patch] tuple. In other words, this allows patch and minor updates for versions 1.0.0 and above, patch updates for versions 0.X >=0.1.0, and no updates for versions 0.0.X.

And the npmjs.com semver calculator confirms that.

Npm is fun!

peppsac

It looks good to me but I'll leave the final word to you or @tbroyer on this topic :)

(nit: there's a typo to correct in one commit title: dependencie-> dependency)

jsdoc was failing on the newest node8 release, probably because some api change. see https://github.com/jsdoc3/jsdoc/releases/tag/3.5.5

autra · 2017-10-16T09:26:35Z

(nit: there's a typo to correct in one commit title: dependencie-> dependency)

indeed, corrected :-)

autra · 2017-10-16T10:29:48Z

Wait before merging, there's actually a problem by doing so: in a project depending on itowns, three would get installed and imported 2 times: one in node_modules/three, and one in node_modules/three/itowns/node_modules/three, causing a lot of problem (Object3D's id being wrong, code using foo instanceOf Vector3 becoming unreliable etc).

We completely fall in the use case of peerDependencies: we are a library using another one (three), and we also expect our user to use three independently (to add their own object for instance).

as advised in a console warning.

eslint-config-airbnb-base requires a peer dependency on eslint-plugin-import v2.7. We had it in version v2.2.

tbroyer · 2017-10-16T10:55:31Z

there's actually a problem by doing so: in a project depending on itowns, three would get installed and imported 2 times: one in node_modules/three, and one in node_modules/three/itowns/node_modules/three, causing a lot of problem

Yes, that's because it's still a "normal" dependency.

A "peer dependency" would fix this, at the expense of asking everyone to explicitly add a dependency on tree (a breaking change, but possibly not a big deal; I mean, everyone using React in the browser for instance knows they have to depend on both react and react-dom; whereas react-dom could have had a "normal" dependency on react, but use a peer dependency instead, probably for the same reasons we'd like to use a peer dependency for three too)

Actually broken; three needs to be removed from "dependencies"

autra · 2017-10-16T10:58:21Z

Yep, but actually, I think I have now the good setup: having them both in peerDependencies and devDependencies (would get installed too on npm i in itowns).

Yes, we actually don't want to hide THREE at all, and I don't think it's bad to require users to install it separately. That also means we should remove itowns.THREE someday.

peppsac · 2017-10-16T11:00:20Z

That also means we should remove itowns.THREE someday

Isn't this PR a good time to get rid of itowns.THREE?

autra · 2017-10-16T11:47:52Z

Isn't this PR a good time to get rid of itowns.THREE?

I need to think of a way to keep being able to use three in examples then.

tbroyer · 2017-10-16T14:18:12Z

Use another entrypoint script than Main.js when bundling with Webpack?
that reexports everything from Main.js and three and proj4 (btw, should we do the same for proj4 that we do with three in this PR?); that way, NPM users would use Main.js (modified to remove three) and their local/peer three, and examples (and those people using the Webpack bundle) would have to use itowns.THREE.

The alternative would be to have the bundle depend on an "ambient" three (i.e. force users of the bundle to first include three in the page – like people are used to include jquery before they can use any jquery plugin).

BTW, there seems to be a single use of itowns.THREE outside of examples (in utils/debug/PointCloudDebug.js)

autra · 2017-10-20T15:26:37Z

(btw, should we do the same for proj4 that we do with three in this PR?)

Done.

Isn't this PR a good time to get rid of itowns.THREE?

Well, that's really not that simple actually. I need to persuade itowns.js to take THREE (and all our peerDependencies really) as an external. It's quite easy of course, but it also means itowns.js won't include our peerDependencies by default. In turns, this means that if we want to still provide a drop-in bundle, I need to generate a itowns_full.js too. I have something that nearly works, but currently it involves adding 2 more webpack configs :-) Worth a PR on its own, right?

autra · 2017-10-20T15:27:09Z

I just pushed a commit that put proj4 in peerDependencies. @iTowns/reviewers please r?

tbroyer

Well, that's really not that simple actually.

How about my proposal to have the "drop-in bundle" ship with three and proj4 and provide them at itowns.THREE and itowns.proj4? (exactly like we do today; just that those properties wouldn't exist when using itowns as an npm dependency)

Worth a PR on its own, right?

I'm fine with this (though if you'd like to go with the –probably simpler– proposal above, then maybe do it in this PR)

autra · 2017-10-20T15:44:13Z

I didn't want to bother exposing itowns.THREE and itowns.proj4 for user of the full bundle, so I was planning to completely remove these from itowns for everybody. Those wanted to use them directly would either use npm or our "light" bundle. WDYT?

autra · 2017-10-20T15:44:26Z

no strong opinion either way though.

autra · 2017-10-23T16:40:35Z

@tbroyer I ended up implementing your suggestion. I have been too greedy at first: I wanted a clean module use case and a full bundle and a bundle relying on ambiant peer dependencies, but that's apparently impossible to do with one webpack configuration. So let's be more reasonable here :-) WDYT now?

I still might do an ambient build (without peerDependencies) one day if we feel the need (and I find the motivation) for it. Thanks for your suggestion anyway :-)

@iTowns/reviewers updated.

When people uses a bundler, they can directly import these no need to pollute our namespace with itowns.THREE, proj4 etc. However, for people using our bundle, we still expose those peerDependencies, are they are included in it anyway. In the future, we might provide 2 bundles: one full bundle like it is today, and a bundle relying on ambiant peerDependencies (attached to the windows object).

autra · 2017-10-23T17:44:41Z

webpack.config.js

@@ -54,7 +54,7 @@ module.exports = {
                // .babelrc is used for transpiling src/ into lib/ in the prepublish
                // phase, see package.json
                options: {
-                    presets: ['es2015'],
+                    presets: [['es2015', { modules: false } ]],


Babel support for export * from 'foo' seems broken, so I let webpack deals with that.

peppsac

LGTM

autra requested a review from tbroyer October 13, 2017 14:37

autra mentioned this pull request Oct 13, 2017

Plugins ? #529

Closed

tbroyer previously approved these changes Oct 13, 2017

View reviewed changes

peppsac reviewed Oct 14, 2017

View reviewed changes

chore(packaging): Update jsdoc to correct incompatibility with node8

bfe746f

jsdoc was failing on the newest node8 release, probably because some api change. see https://github.com/jsdoc3/jsdoc/releases/tag/3.5.5

autra force-pushed the fix_npm_package branch from dbe4478 to 7c27899 Compare October 16, 2017 09:26

autra added 3 commits October 16, 2017 12:40

chore(packaging): put three and three.meshline into peerDependencies

9dc3dc4

We completely fall in the use case of peerDependencies: we are a library using another one (three), and we also expect our user to use three independently (to add their own object for instance).

chore(test): set BABEL_DISABLE_CACHE=1 for unit test too

518697d

as advised in a console warning.

chore(packaging): correct a bad peer dependency

19a5c97

eslint-config-airbnb-base requires a peer dependency on eslint-plugin-import v2.7. We had it in version v2.2.

autra force-pushed the fix_npm_package branch from 7c27899 to 19a5c97 Compare October 16, 2017 10:46

fix(packaging): put proj4 into peerDependencies and devDependencies

37fb058

tbroyer approved these changes Oct 20, 2017

View reviewed changes

autra force-pushed the fix_npm_package branch from 4573f88 to 4af3aa9 Compare October 23, 2017 17:37

autra commented Oct 23, 2017

View reviewed changes

peppsac approved these changes Oct 24, 2017

View reviewed changes

tbroyer approved these changes Oct 24, 2017

View reviewed changes

autra merged commit 357ed7b into master Oct 24, 2017

autra deleted the fix_npm_package branch October 24, 2017 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix npm package #532

Fix npm package #532

autra commented Oct 13, 2017

tbroyer left a comment

autra commented Oct 13, 2017

tbroyer commented Oct 13, 2017

autra commented Oct 13, 2017

peppsac left a comment

autra commented Oct 16, 2017

autra commented Oct 16, 2017

tbroyer commented Oct 16, 2017

autra commented Oct 16, 2017

peppsac commented Oct 16, 2017

autra commented Oct 16, 2017

tbroyer commented Oct 16, 2017

autra commented Oct 20, 2017

autra commented Oct 20, 2017

tbroyer left a comment

autra commented Oct 20, 2017

autra commented Oct 20, 2017

autra commented Oct 23, 2017

autra Oct 23, 2017

peppsac left a comment

Fix npm package #532

Fix npm package #532

Conversation

autra commented Oct 13, 2017

Description

tbroyer left a comment

Choose a reason for hiding this comment

autra commented Oct 13, 2017

tbroyer commented Oct 13, 2017

autra commented Oct 13, 2017

peppsac left a comment

Choose a reason for hiding this comment

autra commented Oct 16, 2017

autra commented Oct 16, 2017

tbroyer commented Oct 16, 2017

autra commented Oct 16, 2017

peppsac commented Oct 16, 2017

autra commented Oct 16, 2017

tbroyer commented Oct 16, 2017

autra commented Oct 20, 2017

autra commented Oct 20, 2017

tbroyer left a comment

Choose a reason for hiding this comment

autra commented Oct 20, 2017

autra commented Oct 20, 2017

autra commented Oct 23, 2017

autra Oct 23, 2017

Choose a reason for hiding this comment

peppsac left a comment

Choose a reason for hiding this comment