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

Make the distribution build the main NPM entry point #3551

Closed
mourner opened this issue Nov 7, 2016 · 4 comments · Fixed by #4423
Closed

Make the distribution build the main NPM entry point #3551

mourner opened this issue Nov 7, 2016 · 4 comments · Fixed by #4423

Comments

@mourner
Copy link
Member

mourner commented Nov 7, 2016

Currently our readme suggests require('mapbox-gl') for Browserify-bundled projects and require('mapbox-gl/dist/mapbox-gl.js') for all the rest.

But is there a real need to support building Mapbox GL JS from within a Browserify-bundled project? Would it be better overall to just change the main entry point in the package.json to dist/mapbox-gl-dev.js and make the workflow the same (require('mapbox-gl')) for all module systems?

This would also enable us to safely use a Yarn lockfile in the repo (#3364 ) for producing the same build and not worry about any downstream modules, and also move all dependencies into devDependencies for a lighter downstream install.

The only cons I can think of:

  • Possible code duplication in downstream Browserify-bundled modules that also use some mapbox-gl dependencies like geojson-vt or earcut. Very rare and not a big deal.
  • Not being able to require a specific branch/hash of mapbox-gl in a Browserify-bundled downstream module. Not a big deal as well.

Thoughts @jfirebaugh @lucaswoj @tmcw?

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 7, 2016

I think this is a good idea.

Do you have a sense of what precedent exists for similar libraries?

Not being able to require a specific branch/hash of mapbox-gl in a Browserify-bundled downstream module. Not a big deal as well.

We could build system that ensures dist/mapbox-gl.js is up to date for every commit. A combination of a git precommit hook + a CI test should do the trick. We don't need to do this until we feel the pain of not being able to require a specific branch / hash.

@mourner
Copy link
Member Author

mourner commented Nov 7, 2016

We could build system that ensures dist/mapbox-gl.js is up to date for every commit.

We should avoid committing the build to the repo at all costs. I think it's much easier to just publish a prerelease version in case we have to require a particular version from a downstream module.

@jfirebaugh
Copy link
Contributor

We should do this. Bug reports from webpack users who are doing require('mapbox-gl') is our most common support issue at the moment.

@anandthakker
Copy link
Contributor

anandthakker commented Jan 20, 2017

Not being able to require a specific branch/hash of mapbox-gl in a Browserify-bundled downstream module. Not a big deal as well

If we're going to make 'dist/mapbox-gl.js' the main script for the NPM package, then I'd like to advocate that we also support require('mapbox-gl/js/mapbox-gl.js') as a way to target a specific commit in a browserify build. (Alas, this would mean not moving build deps to devDependencies.)

The occasions when it's necessary to target a specific commit of a library may be infrequent, but I think they can be sort of severe or urgent: it's the kind of thing where you're fighting some subtle bug for hours trying to ship something and finally the only workaround involves temporarily forking/patching the lib you're depending on and installing your fork. It's invaluable in those cases to be able to do npm install github:myfork/mapbox-gl-js#quick-fix

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

Successfully merging a pull request may close this issue.

4 participants