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

Overhaul build process #65

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

rowanwins
Copy link

This PR basically overhauls the build setup for georaster

  • Moves from webpack to rollup (which is better suited to bundling libraries)
  • Creates a number of different packages to target different environments.
    • Now only the jsbundle is heavy as it includes geotiffjs. This is the one you'd use via a CDN.
  • Updates geotiff dependency
  • Creates seperate entry points for node vs the browser (so cross-fetch isn't included in the browser version)

This probably still needs a bit of testing to make sure nothing is broken but 🤞 I reckon it's a pretty good start.

@DanielJDufour
Copy link
Member

awesome! Having a tool like rollup meant for making multiple builds would be great! You'll have to forgive me because I haven't used rollup before, so I probably have some naive questions.

My main concern is just making sure we don't break any current implementations, so I have a few questions in mind:

Are all the current builds found in https://unpkg.com/browse/[email protected]/dist/ still there, (for situations where people are directly referencing the build files through a script tag)?

Could dist/jsbundle/georaster.bundle.js move to dist/georaster.browser.bundle.js anddist/jsbundle/georaster.bundle.min.js move to dist/georaster.browser.bundle.min.js?

Can people still require("georaster") in a CJS node environment? I just want to to make sure people don't have to use dynamic imports.

Really asking because of my lack of knowledge, but what is the "exports" key in the package.json? Is that something used by rollup or other tools?

I use GeoRaster in another project, can I still make a build with the worker inlined? (Probably a silly question, but I just want to make sure we aren't prevent other projects from creating single file builds)

Any other significant changes or implications on how people implement georaster?

If I want to require georaster in node, can I require a file directly or do I have to require the build?

Otherwise, I think we can cut a georaster@next release and test integrating with that. Can the browser/jsbundle builds have a .map file? Not a strict requirement, because it seems we don't currently have one, but a nice to have :-)

Thoughts?

Thank you! I learned a lot about rollup just looking at your code!

@rowanwins
Copy link
Author

I've made a few little tweaks to the config based on your requests @DanielJDufour

  • worker is now inlined
  • some of the bundle files are now in a new name/location
  • source maps added

Are all the current builds found in https://unpkg.com/browse/[email protected]/dist/ still there, (for situations where people are directly referencing the build files through a script tag)?

So I think you'd publish this as a new major version to prevent any accidental breakages in terms of requiring non-existant/moved files.

Can people still require("georaster") in a CJS node environment? I just want to to make sure people don't have to use dynamic imports.

Yep - so that should people ppl to dist/node/georaster.js which the main field in the package.json points to.

If I want to require georaster in node, can I require a file directly or do I have to require the build?

The default approach in node should automatically direct ppl to the node-compatible-build, but if really necessary people could still do something like require('georaster/src/index.node.js'), but I don't think it would be necessary.

... what is the "exports" key in the package.json? Is that something used by rollup or other tools?

This is a new-ish NodeJS feature for packaging. It's probably not entirely necessary here, but in theory it allows you export multiple modules (which could be handy for shipping node vs browser user modules)... now that I think about it maybe that's something to explore further...

Thank you! I learned a lot about rollup just looking at your code!

No worries. This is a more complex package because of the node vs browser differences (with cross-fetch) so it's using a few different output types. But once you get going with rollup I find it easier than webpack to understand how to configure and create differing outputs. And for simple libraries (eg this one of mine) it's really straight forward.

Two final things

  1. For having a play with how the bundle is working you should be able to checkout this branch with git, and then use npm link to see how it performs in external applications (eg a node app, vs a es6 browser app etc). Here is a nice handy tutorial on npm link. I find npm link super-handy when I'm working on multiple libraries and/or apps which depend on one another.

  2. I also need to do is look at whether we need to pass things through babel or not. From my quick scan of the code you don't have any non-standard syntax so the build process shouldn't need to go through babel, but it would be good to confirm

@elliots
Copy link
Contributor

elliots commented Jul 28, 2023

Thankyou for this, it removes 392(!) dependencies. I did get it to work, but only after disabling the web worker due to it not being able to find geotiff. (ReferenceError: geotiff is not defined)

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