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

Use dynamic imports for non-browser-compatible modules #422

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

Conversation

IvanSanchez
Copy link

Bit of background: I'm at the Évora OGC-OSGeo-ASF codesprint, trying to get multiband rasters work with gleo via geotiff.js. I wrote gleo so that it worked as native browser ESM modules, via <script type='module'> and importmap. See e.g. https://gitlab.com/IvanSanchez/gleo/-/blob/master/browser-demos/91-offscreen-indicator.html?ref_type=heads#L19-L40

The problems that turn up when trying to run geotiff.js as native ESM modules are pretty much the sames as #411 - a file like sources/remote.js unconditionally imports the browser-compatible and the browser-incompatible files (i.e. a file with import from 'fs' or import from 'http' will crash in a browser).

The approach of this fix is to conditionally load those troublesome modules, using dynamic imports. It still works the same, just with a couple more async calls.

See also DanielJDufour/xml-utils#7

Signed-off-by: Iván Sánchez Ortega <[email protected]>
Side effect of the xml-utils changes - apparently babel tries to jump in
 and polyfill stuff.

Signed-off-by: Iván Sánchez Ortega <[email protected]>
Signed-off-by: Iván Sánchez Ortega <[email protected]>
Signed-off-by: Iván Sánchez Ortega <[email protected]>
@constantinius
Copy link
Member

Thanks @IvanSanchez for providing this. This was always a thorn in my side (having to reference modules not available for the browser).

Unfortunately, I'm too far removed from the current state of the art in terms of bundling.

@ahocevar I think you were the one who introduced rollup in the build process. Do you think this change is reasonable?

Copy link
Contributor

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Makes sense and looks good.

@DanielJDufour
Copy link
Contributor

@IvanSanchez , I've promoted your changes for xml-utils from a release candidate to a minor release (along with some upgraded dependencies). If you like, you can use v1.8.0 of xml-utils in this PR now: https://github.com/DanielJDufour/xml-utils/releases/tag/v1.8.0

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.

4 participants