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

Chore: Build node workers as ES modules #2791

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Nov 17, 2023

@belom88 Is it possible to test with this branch?

@ibgreen ibgreen marked this pull request as ready for review November 18, 2023 11:35
@ibgreen ibgreen requested a review from belom88 November 18, 2023 11:35
@belom88
Copy link
Collaborator

belom88 commented Nov 20, 2023

This is exactly what I've reverted in #2790 . We can probably revert *.cjs back to *.js extensions but --format=esm is exactly what is breaking the tile converter right now.
cjs/js is about what is working in NodeJS by default. #2775 seem for me a breaking commit.

@belom88
Copy link
Collaborator

belom88 commented Nov 20, 2023

The error is "Worker exception: Error: Dynamic require of "stream" is not supported"

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 20, 2023

The error is "Worker exception: Error: Dynamic require of "stream" is not supported"

Can you pintpoint the line where this is happening? Since I can't find it in the source code I am assuming it is happening in the built code. But if we are building with --format=esm, seems that require shouldn't be emitted, so it is a mystery error?

@belom88
Copy link
Collaborator

belom88 commented Nov 20, 2023

The error is "Worker exception: Error: Dynamic require of "stream" is not supported"

Can you pintpoint the line where this is happening? Since I can't find it in the source code I am assuming it is happening in the built code. But if we are building with --format=esm, seems that require shouldn't be emitted, so it is a mystery error?

Probably, the root cause is in decoder/encoder libs (modules/draco/src/libs/draco_encoder.js, modules/draco/src/libs/draco_wasm_wrapper.js)

@belom88 belom88 removed their request for review November 20, 2023 12:30
Copy link
Collaborator

@belom88 belom88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tile converter fails with "Worker exception: Error: Dynamic require of "stream" is not supported". Probably draco encoder/decoder libs a not compatible with esm.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 20, 2023

Probably draco encoder/decoder libs a not compatible with esm.

Yes that could be. I suppose if they were renamed to .cjs it might work. I guess in that case we'd need to copy them from the Google CDN.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Mar 28, 2024

@belom88 Can you take another look at this now that it is rebased on the new build system. Do your concerns still apply?

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.

None yet

2 participants