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 type: module for better ES module handling #267

Closed
wants to merge 2 commits into from

Conversation

ahocevar
Copy link
Contributor

@ahocevar ahocevar commented Jan 1, 2022

This pull request makes it easier to use geotiff.js in modern JavaScript environments (ES module capable Node versions, modern bundlers). It is somewhat related to #242.

By importing from 'geotiff/src/geotiff.js' instead of geotiff, consumers can opt in to unconditionally using the native ES module code.

See openlayers/openlayers#13114 (comment).

@ahocevar ahocevar force-pushed the type-module branch 2 times, most recently from 8b1559c to 6145b24 Compare January 2, 2022 10:11
@ahocevar
Copy link
Contributor Author

ahocevar commented Jan 2, 2022

When this is in, I can rework #253 to use tsc to create CommonJS modules for Node (dist-node directory), replacing the build step that currently uses parcel-bundler@1. Then my plan is to add a build step that builds the decoder worker, so it works in any environment without extra configuration. Then finally, the browser build process (dist-browser directory) can be changed to use rollup or vite.

@constantinius
Copy link
Member

@ahocevar Thanks for your effort! I see that including geotiff.js resulted in build errors for dependent libraries such as OpenLayers. As far as I understand the issue, using module will result in the individual source files to be loaded as JS modules. This is a bit confusing, as they are not really meant to be imported anyhow (only their pre-built versions in dist-browser and dist-node) or do I misunderstand something?

In principle I'm fine with the changes, but what does that mean for other dependent projects? Do they have to change their import syntax or built config? If yes, I suggest we aim for a major release, otherwise we could do a minor one.

I'm a little out of the loop in regards to JS build systems. We switched several times already and all had their separate quirks and issues. The biggest one always seems to be related to how the workers are set up and their respective code is split into separate chunks when building.

What do you consider the benefits of rollup or vite in comparison to what we currently have?

@ahocevar
Copy link
Contributor Author

ahocevar commented Jan 2, 2022

@constantinius:

This is a bit confusing, as they are not really meant to be imported anyhow (only their pre-built versions in dist-browser and dist-node) or do I misunderstand something?

The advantage of importing ES modules rather than what's in dist-browser and dist-node is that bundlers can apply treeshaking, reducing the overall bundle size. Also the ES modules from this package are already being used, because package.json has a "module": "src/geotiff.js" entry. The changes in this pull request are mostly to make this work better.

In principle I'm fine with the changes, but what does that mean for other dependent projects? Do they have to change their import syntax or built config?

There should be no need to change anything in existing projects.

I'm a little out of the loop in regards to JS build systems. We switched several times already and all had their separate quirks and issues. The biggest one always seems to be related to how the workers are set up and their respective code is split into separate chunks when building.

My goal here is to remove exactly these quirks, i.e. to ship a package that can be used both standalone and as a dependency without having to deal with workers. Tools like rollup or vite (which also depends on rollup) can simply be better configured than parcel to achieve this. But I see this as a future effort, so other than #242, I did not include it in this pull request.

@ahocevar
Copy link
Contributor Author

ahocevar commented Jan 7, 2022

Closing this due to potential issues with the main export in CommonJS setups. I'll investigate more and provide a less invasive solution.

@ahocevar ahocevar closed this Jan 7, 2022
@ahocevar
Copy link
Contributor Author

ahocevar commented Jan 8, 2022

When #268 is in, I think it would be worthwhile to take the minimal required changes from #242 to fix ESM exports, i.e. renaming the source files to *.mjs and change local imports to use the whole relative path with file extions, and add an exports map to package.json.

@BHDBvde
Copy link

BHDBvde commented Jan 10, 2022

2 questions:
1 Is this also solving the problem with:
Could not resolve import "fs".
More info: This is a problem since 6.6.1, I'm using web-dev-server, @web/dev-server-rollup and @rollup/plugin-commonjs to start my application.
2 Is this already working or do we have to wait for a new geotiff.js version?

@ahocevar
Copy link
Contributor Author

ahocevar commented Jan 10, 2022

This branch was not merged. See #269 for a replacement. But the Could not resolve import "fs" problems should be fixed already. What are your referring to with "since 6.6.1"? The latest version is 1.0.9.

@BHDBvde
Copy link

BHDBvde commented Jan 10, 2022

6.6.1 is the ol version I am using. When using a newer version we will get the resolve import error.

@ahocevar
Copy link
Contributor Author

@BHDBvde If ol@6.11 does not work for you, then #269 won't fix that problem either. In that case, would you be able to share a repository that allows to reproduce your problem?

@BHDBvde
Copy link

BHDBvde commented Jan 13, 2022

Yes I will try to do that.

@BHDBvde
Copy link

BHDBvde commented Jan 21, 2022

@ahocevar For now we fixed this with an empty class for some files: check for node-resolve:empty.js in rollup load function

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