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

Adds rollup configuration #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZacBrownBand
Copy link

@ZacBrownBand ZacBrownBand commented Aug 15, 2018

Adds rollup configuration allowing the library to be built into a single distribution file and adds the ability to support multiple module types.

I needed to make a bundled version to use in a web app and thought it could be useful for others. i added a rollup configuration and added rollup as dev dependency.

To build a umd module:

npm install
npm run-script dist // will run "rollup -c" (rollup with config)

The result will be a new javascript file dist/geodesy.js that contains the geodesy library.

Others may update the rollup.config.js to change the output.

Note: I added a .gitignore and added dist directory to it, as to not change whats in the repo. Ideally the dist would exsist in the published npm package but not the repo.

…gle distribution file and adds the ability to support multiple module types.
@quentinmit
Copy link

Instead of requiring an extra dist step (that adds a node dependency where it might not already be present), can you just change the source files themselves to use UMD instead of the CommonJS module syntax they do now? AIUI, UMD is backwards compatible with CommonJS.

@ZacBrownBand
Copy link
Author

@quentinmit Sorry, I don't follow. What is the draw back of require an npm package to run the build? Changing the source is much more intrusive and limiting.

@quentinmit
Copy link

@ZacBrownBand If someone wants to use this library in their webapp, they'd need to install Node and rollup just to generate the UMD version of the source. If they're writing a webapp, it's entirely possible they don't currently have any dependency on Node at all.

I think it would be a tiny diff to the source to make this be natively UMD - basically a couple lines per source file. Not much worse than what you're already doing in this pull request, no?

@quentinmit
Copy link

Specifically, I'm suggesting the current Node-specific header and footer get replaced with the first header and footer here instead:

https://github.com/umdjs/umd/blob/master/templates/returnExports.js

@ZacBrownBand
Copy link
Author

@quentinmit
Feel free to implement that and open a pr. This pr is to add a webpack config to allow people who are using node/npm to pull this package and build it.

@ZacBrownBand
Copy link
Author

@chrisveness Was there an issue with this pr? Or is it just not something you are interested in having included in this project?

@chrisveness
Copy link
Owner

Umm – yes and no, I think. I didn't intend to close it, GitHub appears to have closed it as a result of me renaming branches.

My concerns are:

  • if I am to cater for bundlers, should I not also cater for webpack, parcel, and perhaps others?
  • I don't have experience using bundlers – I prefer to load from CDNs, especially given the advantages of HTTP/2
  • most significantly, the version 2 of the library now uses ES-modules, which I think reduces the value of bundlers? (especially using HTTP/2-capable CDNs).

As it stands, the library can be used without a build step (see example usages). As I don't use bundlers, I don't know how well it works for those using webpack or rollup – can that not be achieved with the library as it stands? (either v1/CJS or v2/ES-modules).

@chrisveness chrisveness reopened this Apr 10, 2019
@ZacBrownBand
Copy link
Author

Ah, I have not looked a the v2 with es modules, that is exciting.

Maybe the need to have the library available as a single file bundle is something many people need. My aim was to add the option of bundling without adding the requirement to bundle, but with the release of v2 I would assume these changes are now out of date.

Feel free to close, If you decide to implement some sort of bundling option (here or in npm packages) let me know if you'd want some collaboration.

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