Fix package import for modern frontend #144

PacoDu · 2020-04-13T17:12:25Z

This PR fixes issues I introduced with #131 :

I removed the browser field from package.json and added the module field pointing to src/main.js. The browser field was misleading builders like webpack that were using the built version of the application (dist-browser), breaking tree-shaking and imports from src. (see: browser vs module fields in package.json webpack/webpack#4674 about webpack's package.json fields priority)
I also removed the import 'threads/register' which was breaking webpack builds
Add files to package.json
Add jsdelivr to package.json for CDN installation and usage of GeoTIFF via the global variable
Enhance README for installation and usage (also add CDN installation)
Add documentation about webpack build requirement for the worker Pool to work properly
Parallel build for browser and node with npm-run-all

I created a repo to do some tests with treads.js packaged in a library: https://github.com/PacoDu/threads-package. I've tested the solution with multiple clients: VueJS, React, NodeJS, raw webpack and raw parcel bundling. It seems to work properly in every cases.

This PR may needs more tests, but it's better than the current version that is not working as expected for modern frontend builds.

Should fix: #143

… Pool is used

philip-firstorder · 2020-04-13T17:57:07Z

I get this error in Angular 9:
"export 'default' (imported as 'GeoTIFF') was not found in 'geotiff'

README.md

constantinius · 2020-04-13T18:20:13Z

@PacoDu thanks for following up on this!

Webpack allowed to inline workers, is there a similar functionality for parcel as well?

Regarding the package.json fields: it seems like the browser field does not work well for webpack, so I guess using module instead would be fine, but doesn't this conflict with other users of that field?

Thanks for testing with so many frameworks, that is good and necessary!

philip-firstorder

@PacoDu thanks for following up on this!

Webpack allowed to inline workers, is there a similar functionality for parcel as well?
No idea, I just use geotiff in Angular 9, I am building now a demo project for you to add in
https://github.com/PacoDu/threads-package

Regarding the package.json fields: it seems like the browser field does not work well for webpack, so I guess using module instead would be fine, but doesn't this conflict with other users of that field?
module should be ok

Thanks for testing with so many frameworks, that is good and necessary!
Sorry I tested just Angular, but I have used this library for a year now in production with drone COGs

README.md

philip-firstorder · 2020-04-13T19:17:28Z

I created an angular universal demo repository for you to reproduce the error and test:
https://github.com/firstorder-gmbh/geotiff-angular

To ger the error "export 'default' (imported as 'GeoTiff') was not found in 'geotiff' open a terminal and run:
npm i && npm run build:ssr

First will install the packages, including your PacoDu:fix-modern-frontend-import fork
Then it will build angular universal ssr for Server Side Rendering.

ilan-gold · 2020-04-13T19:34:16Z

@PacoDu I tested the build in my component library using npm pack on this branch locally as well as taking in this branch's code directly from git in the package.json how @philip-firstorder has in his testing repo (but I use React so can't comment on the rest of any issues he may be having). It worked great for me with the webpack plugin. I then bundled my package with rollup and was able to install it in another project and have it work with your branch as a dependency. This works well for me as well. I would give this the all clear from my end.

PacoDu · 2020-04-13T20:10:18Z

I get this error in Angular 9:
"export 'default' (imported as 'GeoTIFF') was not found in 'geotiff'

It's not the first time I see this error with this repo, I'm pretty sure it's related to the way main.js exports the package: It doesn't explicitly use export default but uses export * on geotiff.js. This solution works if you use a transpiler like babel or esm but doesn't work with NodeJS native ESM or apparently typescript based on your feedback.

@constantinius I don't really see any reason to keep main.js it doesn't add any functionality, and if we set geotiff.js as the main file of the package it should solve the issue. An other solution would be to import GeoTIFF and add the export default in the main file. Let me know which solution suits you.

PacoDu · 2020-04-13T20:27:14Z

I've also introduced a parallel build with npm-run-all, it reduces the build time.

PacoDu · 2020-04-13T20:37:45Z

I created an angular universal demo repository for you to reproduce the error and test:
https://github.com/firstorder-gmbh/geotiff-angular

To ger the error "export 'default' (imported as 'GeoTiff') was not found in 'geotiff' open a terminal and run:
npm i && npm run build:ssr

First will install the packages, including your PacoDu:fix-modern-frontend-import fork
Then it will build angular universal ssr for Server Side Rendering.

@philip-firstorder Thanks for providing the test repo. The issue is not related to the one I mentioned above. You used GeoTiff instead of GeoTIFF.
But the SSR build keeps failing due to NodeJS specific modules like fs, http, etc. I'm not using AngularJS and never used typescript, do you have any idea how to prevent this kind of error ?

PacoDu · 2020-04-13T20:52:51Z

Well I probably had some cache or link issue with my previous comment, we also need to change the main file to geotiff.js instead of main.js to fix the typescript import as I mentioned above.

Changing "main": "src/main.js" to "main": "src/geotiff.js" in the package.json solves the default import error.

PacoDu · 2020-04-13T21:24:24Z

Note: This comment assumes that the main entrypoint of geotiff.js is set to src/geotiff.js.

@philip-firstorder I did some tests with your repo. What I don't understand is that the ssr build of angular is using the node js entrypoint ... But it is not able to resolve node specific modules like fs, http, https ...

You can add the following browser field to your package.json to properly disable the NodeJS modules in your SSR build:

"browser": {
    "fs": false,
    "http": false,
    "https": false
 },

Also, it is really strange how angular resolves the module: It looks for the module entrypoint to resolve the imports but uses the node entrypoint (main) at some point because webpack emits the following warning at the end:

WARNING in ../geotiff.js/dist-node/main.js 1:292-296
Critical dependency: the request of a dependency is an expression

🤔🤔🤔

See: angular/angular-cli#8272

The resulting build is probably not optimal due to webpack warning, but it passes.

philip-firstorder · 2020-04-13T21:45:33Z

I renamed the import from GeoTiff vs GeoTIFF. But still get the error:
error "export 'default' (imported as 'GeoTIFF) was not found in 'geotiff'

The only way to get rid of this error was to change line: 86 in node_modules/geotiff/package.json
to:

"module": "src/geotiff.js",

I added these lines in package.js to get rid of the fs, http, https errors. However I don't feel that I have to add these at all. Shouldn't geotiff.js take care to do the proper browser checks?

"browser": {
    "fs": false,
    "http": false,
    "https": false
 },

As for the warning message I tried adding a browser check in an if clause before creating any GeoTIFF.Pool() instance:

pool = typeof window !== 'undefined' ? new GeoTIFF.Pool() : null;

But I still get the warning:

WARNING in ../geotiff.js/dist-node/main.js 1:292-296
Critical dependency: the request of a dependency is an expression

So the current pull request is not yet working in angular.

PacoDu · 2020-04-13T21:52:09Z

Well, regarding this PR I think we are good to go. I can address the main entrypoint issue in another PR and we may need to investigate SSR builds. I'll also do some tests with VueJS SSR.

constantinius · 2020-04-14T16:11:05Z

README.md

 <script>
  console.log(GeoTIFF);
+  // Note: GeoTIFF.Pool will not work


Why is that exactly? The worker is not available on jsdelivr? Can we upload/cache just one file there?

Because the worker has its own bundle, and it is not possible to load a worker from a cross-origin

If you want to be able to import the bundle from a CDN maybe it is possible with webpack. I can try to do this in another PR (a revert is not enough as we still need a separate build for NodeJS and browsers)

Understood.
And we still need a bundled version for node, right?

Yes but it can be achieved with webpack as well.

src/pool.js

PacoDu · 2020-04-14T21:37:21Z

Closing, merged with #145

PacoDu added 7 commits April 13, 2020 02:42

fix: remove browser field from package.json and add module

dd44264

feat: parallel build of browser and node version

0e8e8fd

chore: add files to package.json

24b2f16

fix: remove threads/register that was breaking webpack builds

b080d5b

feat: add jsdelivr for CDN usage

a9b33dc

chore: enhance README install and usage

069eab1

chore: add documentation about requirements for webpack if the worker…

ee76a94

… Pool is used

PacoDu mentioned this pull request Apr 13, 2020

Unable to resolve module "geotiff" when importing in React native #126

Open

philip-firstorder reviewed Apr 13, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

philip-firstorder reviewed Apr 13, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

chore: fix typo in README.md

730f7df

fix: sync package-lock.json

668d245

PacoDu mentioned this pull request Apr 13, 2020

Change the main entrypoint to geotiff.js #145

Merged

constantinius reviewed Apr 14, 2020

View reviewed changes

PacoDu closed this Apr 14, 2020

PacoDu mentioned this pull request Apr 28, 2020

Threads.js Bundling #150

Open

Fix package import for modern frontend #144

Fix package import for modern frontend #144

Uh oh!

Conversation

PacoDu commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philip-firstorder commented Apr 13, 2020

Uh oh!

Uh oh!

constantinius commented Apr 13, 2020

Uh oh!

philip-firstorder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philip-firstorder commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilan-gold commented Apr 13, 2020

Uh oh!

PacoDu commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PacoDu commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PacoDu commented Apr 13, 2020

Uh oh!

PacoDu commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PacoDu commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philip-firstorder commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PacoDu commented Apr 13, 2020

Uh oh!

constantinius Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

PacoDu Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

PacoDu Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constantinius Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

PacoDu Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PacoDu commented Apr 14, 2020

Uh oh!

Uh oh!

PacoDu commented Apr 13, 2020 •

edited

Loading

philip-firstorder left a comment •

edited

Loading

philip-firstorder commented Apr 13, 2020 •

edited

Loading

PacoDu commented Apr 13, 2020 •

edited

Loading

PacoDu commented Apr 13, 2020 •

edited

Loading

PacoDu commented Apr 13, 2020 •

edited

Loading

PacoDu commented Apr 13, 2020 •

edited

Loading

philip-firstorder commented Apr 13, 2020 •

edited

Loading

PacoDu Apr 14, 2020 •

edited

Loading