-
Notifications
You must be signed in to change notification settings - Fork 183
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
Implement threads.js and parcel-bundler #131
Conversation
chore: upgrade dependencies
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
Thanks ;)
|
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.
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? |
I'm not sure about what you mean by
Parcel is only a bundler and is not related to tests |
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 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/predictor.js
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this form:
if (i * stride * width * bytesPerSample >= block.byteLength) {
break;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eslint rules related. We should either change the rule, add curly braces or use the inline syntax.
https://eslint.org/docs/rules/curly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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
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. |
Awesome, I like that even better.
Hm, why not? And how could we translate the codebase to pure ESM?
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. |
I'll do some adjustment for eslint rules (curly), one last review of the changes and I think we are good to go ! |
@PacoDu from my point of view this looks very good. If you agree, we could merge this PR now. |
Sorry for the delays, I think we are good to go. 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. |
Alright, I think we can stick with what we've got. |
Can you make a version bump and publish the package ? Thanks ! |
new version is published (and automatic building too) |
@PacoDu I think something is missing for building: https://travis-ci.org/github/geotiffjs/geotiff.js/builds/671327370#L1405 Do you have an idea? |
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. |
I may have misunderstood the error here, I don't see why this is related to the build command ... |
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 ... Could we try to retrigger the CI ? |
I never used travis but I think I've found the issue here: Wouldn't it be required for the deploy step to keep |
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 |
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: I think it is fair to stick to v12. |
This should be solved now that you rebuilt the I recommend using npm version to upgrade the package version (it automatically set |
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? |
This occurs when the pre-built binary is not available. For travis CI setting the version to |
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. |
Oh, sorry for the trouble, what was broken ? |
I'm not sure when it happened, but the whole API reference is pretty broken and unusable. I'll try to fix it. |
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.Bundle size comparison:
Fixes #53
Fixes #71 by adding
set -e
tosetup_data.sh
to force script failureCloses #115