Implement threads.js and parcel-bundler #131

PacoDu · 2020-03-09T22:52:38Z

This one is quite a long shot, I've implemented thread.js for the decoder.worker.js (#115) which was the main issue about the package bundling (and babel configuration). I also changed the package bundler to parcel-bundler which is much faster and simpler in my opinion.

Introduce two separate builds for browser and nodejs which, i think, should solve most of the issues about importing geotiff.js (Can't resolve 'fs' in dist #98 Unable to resolve module "geotiff" when importing in React native #126 import is not use，i have import geotiff，but when i use its function ，its not defined! #110 Error: Cannot find module 'babel-runtime/helpers/possibleConstructorReturn' #74 Cannot require dist/geotiff.js #23 Remove babel-cli from dependencies #43)
Upgrade and cleanup dependencies
Lint and tests fixes

Bundle size comparison:

Browser:
- before: 690KB
- after: 316KB
Node:
- before: 690KB
- after: 55.13KB

Fixes #53
Fixes #71 by adding set -e to setup_data.sh to force script failure
Closes #115

chore: upgrade dependencies

constantinius · 2020-03-10T09:35:41Z

@PacoDu

Wow, that is quite a PR! Thank you very much for providing this. I quickly skimmed over the changes and think it is very sound, although I'm no expert in many cases.

I will do a more detailed review this week (will be mostly further questions), and try to merge this as soon as possible.

Thank you very much (again) for the effort!

…m install, expose GeoTIFF as global variable for browser build

PacoDu · 2020-03-10T22:24:10Z

Thanks ;)
Some notes about the PR:

I wasn't able to determine test/lzw.html purpose, it's not related to any existing source code for the imports. It's commented out for now has it blocks parcel dev server otherwise
Threads.js is not working in mocha test so the worker test i wrote is not executed. I couldn't find why but the Pool is properly working once built

constantinius · 2020-03-11T08:40:11Z

I wasn't able to determine test/lzw.html purpose, it's not related to any existing source code for the imports. It's commented out for now has it blocks parcel dev server otherwise

Thats fair. LZW was developed as a PR, the developer added that html file. I don't think it is necessary to keep it, LZW is tested elsewhere.

Threads.js is not working in mocha test so the worker test i wrote is not executed. I couldn't find why but the Pool is properly working once built

Okay. Having a clean build for node, browser and testing was always more of a chore than it should be. Especially when using threads/workers. Maybe there is some way to test that integrates parcel?

PacoDu · 2020-03-12T22:20:38Z

I'm not sure about what you mean by

Maybe there is some way to test that integrates parcel?

Parcel is only a bundler and is not related to tests

PacoDu · 2020-03-12T23:55:56Z

I took a look about refactoring geotiff.js to native ES6 module in order to be able to load geotiff.js from source in node > 13.2. Currently OSS community seems to be working on upgrading tools and libraries for node native ESM but it doesn't seem quite ready yet. For example thread.js is currently not working with the native ESM module loader of node.js (see andywer/threads.js#220), and it's only one of the issues I encountered will trying to upgrade geotiff.js to native es6 module. That was my first attempt to write ESM modules in node.js but I think I'll wait a bit before retrying, I'll stick to CommonJS for now 😅

constantinius

There are just some minor issues (styling mostly), apart from that I really like the work that has been done here.

Some additional questions would be good to have an answer for:

How does the bundling work where geotiff.js is a dependency, such as the COG-explorer? Does it re-bundle the bundled version of geotiff.js? How would tree-shaking work? Is it still possible to import the source itself?
Just recently added was the analyzer of the webpack bundle. This of course would be removed. Is there perhaps an alternative for that?

.eslintrc.js

README.md

package.json

src/compression/jpeg.js

src/decoder.worker.js

src/pool.js

constantinius · 2020-03-13T13:22:55Z

src/predictor.js

@@ -53,8 +53,7 @@ export function applyPredictor(block, predictor, width, height, bitsPerSample,

  for (let i = 0; i < height; ++i) {
    // Last strip will be truncated if height % stripHeight != 0
-    if (i * stride * width * bytesPerSample >= block.byteLength)
-      break;
+    if (i * stride * width * bytesPerSample >= block.byteLength) break;


I would prefer this form:

if (i * stride * width * bytesPerSample >= block.byteLength) { break; }

eslint rules related. We should either change the rule, add curly braces or use the inline syntax.
https://eslint.org/docs/rules/curly

I see, thanks. I would prefer to use the all rule here. Do you want to add this to your PR since you're already at it? Otherwise, we can do this separately as-well.

test/data/.gitignore

test/dev.js

test/lzw.html

…nsform-runtime

PacoDu · 2020-03-14T03:35:06Z

Just recently added was the analyzer of the webpack bundle. This of course would be removed. Is there perhaps an alternative for that?

I've added the equivalent for parcel ;). The only difference is that it generates the report at the build step, and in the corresponding dist folder.

How does the bundling work where geotiff.js is a dependency, such as the COG-explorer? Does it re-bundle the bundled version of geotiff.js? How would tree-shaking work? Is it still possible to import the source itself?

This depends mostly on the main project configuration. Current sources are unusable without a transpiler because it's not pure ESM. In most cases the main project will import the bundled version, and will target the desired bundle depending on it's configuration by following the main or browser variable of package.json

"main": "dist-node/main.js",
"browser": "dist-browser/main.js",

Sources can only be imported if the main project uses a module loader like babel or esm.

About tree-shaking, it will probably depends on the tree-shaking implementation, but I don't see why it wouldn't work, I'll do some tests to confirm.
If you talk about tree-shaking for geotiff.js, I wanted to enable it but I'm facing an export/import error with threads.js. I'll try to investigate as I think we can still significantly reduce the bundle size with tree-shaking (called scope-hoisting in parcel).

constantinius · 2020-03-16T09:51:58Z

I've added the equivalent for parcel ;). The only difference is that it generates the report at the build step, and in the corresponding dist folder.

Awesome, I like that even better.

Current sources are unusable without a transpiler because it's not pure ESM.

Hm, why not? And how could we translate the codebase to pure ESM?

About tree-shaking, it will probably depends on the tree-shaking implementation, but I don't see why it wouldn't work, I'll do some tests to confirm.
If you talk about tree-shaking for geotiff.js, I wanted to enable it but I'm facing an export/import error with threads.js. I'll try to investigate as I think we can still significantly reduce the bundle size with tree-shaking (called scope-hoisting in parcel).

I've seen the issue, thanks for investigating and raising this issue.

Thank you for providing all the clarifications! I think this is almost ready to be merged (see last items above), if you are of the same opinion.

PacoDu · 2020-03-17T09:35:29Z

I'll do some adjustment for eslint rules (curly), one last review of the changes and I think we are good to go !

constantinius · 2020-03-24T09:44:32Z

@PacoDu from my point of view this looks very good. If you agree, we could merge this PR now.

PacoDu · 2020-03-25T10:24:24Z

Sorry for the delays, I think we are good to go.
Maybe more tests on browsers would be great (I've only tested chrome).

About pure ESM I'll try to take some time to answer but, for what I've experienced while trying to upgrade geotiff.js to pure ESM, this is quite a complicated subject.

constantinius · 2020-03-25T11:26:16Z

Alright, I think we can stick with what we've got.
Thanks again for your contribution! :-)

PacoDu · 2020-03-25T13:44:47Z

Can you make a version bump and publish the package ?
What is the roadmap for 1.0.0 ?

Thanks !

constantinius · 2020-03-25T15:29:50Z

Sure, will do.

What is the roadmap for 1.0.0 ?

Hm, undefined. There are some functionalities that would be really nice like #94, and #135. Also #139 points to a bug on node.

I think I'll make an official roadmap for v1.0.0 using the github project feature.

constantinius · 2020-03-25T16:16:32Z

new version is published (and automatic building too)
Project is up https://github.com/geotiffjs/geotiff.js/projects/1

constantinius · 2020-04-05T18:54:39Z

@PacoDu I think something is missing for building: https://travis-ci.org/github/geotiffjs/geotiff.js/builds/671327370#L1405
Travis CI deploy fails, as it seems that it cannot build the final package for upload.

Do you have an idea?

PacoDu · 2020-04-06T01:38:22Z

npm ERR! file sh
npm ERR! code ELIFECYCLE
npm ERR! errno ENOENT
npm ERR! syscall spawn
npm ERR! geotiff@1.0.0-beta.9 build:browser: `parcel build src/main.js --target browser --out-dir dist-browser/ --global GeoTIFF`
npm ERR! spawn ENOENT

Travis is failing because it's using node v9.11.2 and threads.js requires tiny-worker to work for node <12 (see: https://threads.js.org/getting-started#installation).

We should upgrade node version for Travis CI or decide to support node <12 and add tiny-worker as a dependency.

PacoDu · 2020-04-06T01:40:40Z

I may have misunderstood the error here, I don't see why this is related to the build command ...
I'll take a closer look tomorrow

PacoDu · 2020-04-06T08:41:08Z

Well, that's not related to threads.js, I was misled by https://travis-ci.org/github/geotiffjs/geotiff.js/jobs/671327371#L1292 but there is no tests using the worker pool so node 9.11.2 is not an issue for now.

I pulled the 1.0.0-beta.9 tag from scratch and npm run build was successful locally with node 9.11.2 ... I don't understand why this is failing on travis, also the build command is working at the npm install step : https://travis-ci.org/github/geotiffjs/geotiff.js/builds/671327370#L568

... Could we try to retrigger the CI ?

PacoDu · 2020-04-06T08:51:51Z

I never used travis but I think I've found the issue here:
I see a skip_cleanup: true used for the deploy:pages step:
v1.0.0-beta.8...v1.0.0-beta.9#diff-354f30a63fb0907d4ad57269548329e3R30

Wouldn't it be required for the deploy step to keep node_modules directory ? It seems like node_modules disappeared between the tests and the deploy stage, because the build command is working after npm install (https://travis-ci.org/github/geotiffjs/geotiff.js/builds/671327370#L568) and failing at npm prepare (https://travis-ci.org/github/geotiffjs/geotiff.js/builds/671327370#L1405)

PacoDu · 2020-04-06T12:20:07Z

I think we could jump to node 12 instead of node 10 (to avoid tiny-worker dependency) as it's about to leave LTS in a month: https://github.com/nodejs/Release#release-schedule. Or we could just add a note in the readme for node <12 to install tiny-worker as a dependency if they want to use Pool.

constantinius · 2020-04-06T14:50:43Z

Hrm, I actually set the node version to 12 in the latest build (typo in commit message says v10) but it apparently ran into issues:
https://travis-ci.org/github/geotiffjs/geotiff.js/builds/671558364#L539

I think it is fair to stick to v12.

PacoDu · 2020-04-06T16:29:06Z

This should be solved now that you rebuilt the package-lock.json in your latest commit.

I recommend using npm version to upgrade the package version (it automatically set package.json and package-lock.json values), but maybe you are already using it :p

constantinius · 2020-04-06T16:48:53Z

I recommend using npm version to upgrade the package version (it automatically set package.json and package-lock.json values), but maybe you are already using it :p

I do :-)

Welp, apparently travis insists on re-building node v12 every time a test is triggered, thus resulting in a build time timeout. This does not behave correctly, right?

PacoDu · 2020-04-06T18:05:37Z

This occurs when the pre-built binary is not available. For travis CI setting the version to lts or 13 should do the trick to find a pre-built version. It doesn't matter if node 13 is used to build the package. And we should increase engines.nodes to >=12 in the package.json

constantinius · 2020-04-06T21:51:48Z

Okay, I think the build and deploy now work.

I think that the documentation was broken at some point, and it may be during this PR. (https://geotiffjs.github.io/geotiff.js/index.html)

But I think it needs an overhaul anyways.

PacoDu · 2020-04-07T07:16:08Z

Oh, sorry for the trouble, what was broken ?

constantinius · 2020-04-07T07:44:34Z

I'm not sure when it happened, but the whole API reference is pretty broken and unusable. I'll try to fix it.

PacoDu added 14 commits March 4, 2020 03:03

refactor: implement Pool with threads.js

3c31602

test: add worker pool test

e35f431

refactor: implement pool with threadjs

03c0648

chore: remove worker-loader

829257c

tests: force tests to fail at the right step

f0ba72c

chore: lint

c6da7fa

fix: build

19a56e8

chore: upgrade dependencies

chore: add browser target to package.json

0a9bcdb

chore: configure polyfill with core-js instead of @babel/polyfill

1030807

tests: disable worker test, not working with mocha 7.1

c659360

feat: parcel-bundler working with same features as webpack

e45c517

chore: cleanup webpack artifacts

f5bd519

chore: cleanup and small fixes

f3f7094

chore: remove core-js (not necessary with parcel)

18e72af

PacoDu added 2 commits March 10, 2020 22:58

chore: set prepublish to prepublishOnly to prevent double build on np…

148d933

…m install, expose GeoTIFF as global variable for browser build

doc: update README to match parcel-bundler configuration

a8f2eb1

chore: upgrade thread.js dependency to 1.3.1

6eb6717

constantinius reviewed Mar 13, 2020

View reviewed changes

PacoDu added 6 commits March 14, 2020 02:15

chore: remove regenerator-runtime and swithc back to babel plugin-tra…

2056d48

…nsform-runtime

chore: remove unecessary index from import

5535774

chore: add missing changes to babelrc and main, set dev port to 8090

ab6fd32

chore: remove unused lzw test file

be71193

chore: remove unnecessary *tif from test/.gitignore

d382901

chore: remove eslint fix quirks

2afabed

PacoDu added 2 commits March 23, 2020 16:03

chore: apply eslint rule curly 'all' and brace-style 1tbs

157d0ab

chore: resolve merge conflicts

bbd5a9c

constantinius merged commit d3b0958 into geotiffjs:master Mar 25, 2020

ilan-gold mentioned this pull request Apr 10, 2020

Decoder Worker Reading Bug #143

Closed

This was referenced Apr 13, 2020

Fix package import for modern frontend #144

Closed

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

Open

tschaub mentioned this pull request Oct 5, 2021

Fix linter-identified issues and run the linter before tests #250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement threads.js and parcel-bundler #131

Implement threads.js and parcel-bundler #131

PacoDu commented Mar 9, 2020 •

edited

constantinius commented Mar 10, 2020

PacoDu commented Mar 10, 2020

constantinius commented Mar 11, 2020

PacoDu commented Mar 12, 2020 •

edited

PacoDu commented Mar 12, 2020 •

edited

constantinius left a comment

constantinius Mar 13, 2020

PacoDu Mar 13, 2020 •

edited

constantinius Mar 16, 2020

PacoDu commented Mar 14, 2020

constantinius commented Mar 16, 2020

PacoDu commented Mar 17, 2020

constantinius commented Mar 24, 2020

PacoDu commented Mar 25, 2020

constantinius commented Mar 25, 2020

PacoDu commented Mar 25, 2020

constantinius commented Mar 25, 2020

constantinius commented Mar 25, 2020

constantinius commented Apr 5, 2020

PacoDu commented Apr 6, 2020

PacoDu commented Apr 6, 2020 •

edited

PacoDu commented Apr 6, 2020

PacoDu commented Apr 6, 2020 •

edited

PacoDu commented Apr 6, 2020

constantinius commented Apr 6, 2020

PacoDu commented Apr 6, 2020

constantinius commented Apr 6, 2020

PacoDu commented Apr 6, 2020

constantinius commented Apr 6, 2020

PacoDu commented Apr 7, 2020

constantinius commented Apr 7, 2020

Implement threads.js and parcel-bundler #131

Implement threads.js and parcel-bundler #131

Conversation

PacoDu commented Mar 9, 2020 • edited

constantinius commented Mar 10, 2020

PacoDu commented Mar 10, 2020

constantinius commented Mar 11, 2020

PacoDu commented Mar 12, 2020 • edited

PacoDu commented Mar 12, 2020 • edited

constantinius left a comment

Choose a reason for hiding this comment

constantinius Mar 13, 2020

Choose a reason for hiding this comment

PacoDu Mar 13, 2020 • edited

Choose a reason for hiding this comment

constantinius Mar 16, 2020

Choose a reason for hiding this comment

PacoDu commented Mar 14, 2020

constantinius commented Mar 16, 2020

PacoDu commented Mar 17, 2020

constantinius commented Mar 24, 2020

PacoDu commented Mar 25, 2020

constantinius commented Mar 25, 2020

PacoDu commented Mar 25, 2020

constantinius commented Mar 25, 2020

constantinius commented Mar 25, 2020

constantinius commented Apr 5, 2020

PacoDu commented Apr 6, 2020

PacoDu commented Apr 6, 2020 • edited

PacoDu commented Apr 6, 2020

PacoDu commented Apr 6, 2020 • edited

PacoDu commented Apr 6, 2020

constantinius commented Apr 6, 2020

PacoDu commented Apr 6, 2020

constantinius commented Apr 6, 2020

PacoDu commented Apr 6, 2020

constantinius commented Apr 6, 2020

PacoDu commented Apr 7, 2020

constantinius commented Apr 7, 2020

PacoDu commented Mar 9, 2020 •

edited

PacoDu commented Mar 12, 2020 •

edited

PacoDu commented Mar 12, 2020 •

edited

PacoDu Mar 13, 2020 •

edited

PacoDu commented Apr 6, 2020 •

edited

PacoDu commented Apr 6, 2020 •

edited