geotiffjs / geotiff.js

geotiff.js is a small library to parse TIFF files for visualization or analysis. It is written in pure JavaScript, and is usable in both the browser and node.js applications.
https://geotiffjs.github.io/
MIT License
861 stars 181 forks source link

Use type: module for better ES module handling #267

Closed ahocevar closed 2 years ago

ahocevar commented 2 years ago

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 https://github.com/openlayers/openlayers/issues/13114#issuecomment-994134162.

ahocevar commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

ahocevar commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ahocevar commented 2 years ago

@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 commented 2 years ago

Yes I will try to do that.

BHDBvde commented 2 years ago

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