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

fix: make node workers cjs #2790

Closed
wants to merge 2 commits into from
Closed

Conversation

belom88
Copy link
Collaborator

@belom88 belom88 commented Nov 17, 2023

Workers fail for the tile-converter after v4.0.3.

Worker exception: SyntaxError: Cannot use import statement outside a module
(node:160372) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.

If we are in the package type: "module", another error come:

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

The solution is to make all node workers with *.cjs extension to handle them as common js.

@belom88 belom88 requested a review from ibgreen November 17, 2023 14:34
@ibgreen
Copy link
Collaborator

ibgreen commented Nov 17, 2023

If we are in the package type: "module", another error come

Thanks for looking into this. A quick discussion:

If I have a choice. for node workers, I would prefer focusing on modern apps / es module (type: module), which perhaps just means replacing the dynamic require of stream?

I.e. I would be OK with not having support for CommonJS applications in Node.js

I assume the tile-converter, which is really the only application I know about that uses node workers, is type module?

@ibgreen
Copy link
Collaborator

ibgreen commented Nov 17, 2023

Experimenting with ESM node workers here: #2791, let's see if that approach works first.

@belom88
Copy link
Collaborator Author

belom88 commented Nov 20, 2023

If CommonJS is not preferable format I'd fix the tile converter first. It is broken on the latest version. The order for this issue can be:

  • Merge this hot fix;
  • Make a patch;
  • Find a solution for "Worker exception: Error: Dynamic require of "stream" is not supported";
  • Build workers with flag --format=esm

@ibgreen
Copy link
Collaborator

ibgreen commented Nov 20, 2023

Sounds good, I agree we need to unbreak it asap.

Find a solution for "Worker exception: Error: Dynamic require of "stream" is not supported";

Since there is no dynamic require of stream in the source code, I was assuming that this happens because the node-workers are transpiled to CommonJS, that is why I put up a PR making all node-workers esm. We can look at that once your patch is out.

@belom88 belom88 closed this Dec 15, 2023
@belom88 belom88 deleted the vb/fix-make-node-workers-cjs branch February 7, 2024 16:36
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.

2 participants