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

Keep webpackIgnore comments in pyodide.js #4294

Merged
merged 6 commits into from Nov 17, 2023

Conversation

ryanking13
Copy link
Member

@ryanking13 ryanking13 commented Nov 14, 2023

Description

Resolve #4174

After we changed our build system to esbuild (#3679), /* webpackIgnore */ comments were stripped out after the build.

Since we dynamically import pyodide.asm.js from pyodide.[m]js, this comment is required for webpack bundler (unless we find a better way to deal with this... like merging pyodide.js and pyodide.mjs? I don't know.)

This PR fixes it by putting webpackIgnore comments for every import() statement after building pyodide.js.
A user can do this by adding a webpack plugin, as described in pyodide-webpack-plugin, but I think it is better if we can handle it.

Related:

Checklists

@ryanking13 ryanking13 added this to the 0.24.2 milestone Nov 14, 2023
Copy link
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Thanks @ryanking13!

@ryanking13 ryanking13 merged commit 8dc568f into pyodide:main Nov 17, 2023
40 of 41 checks passed
@ryanking13 ryanking13 deleted the keep-comment branch November 17, 2023 11:33
@hoodmane
Copy link
Member

@mneil What's your thought on webpackIgnore comments? Do you think we should just drop them and encourage people to use your plugin?

@mneil
Copy link
Contributor

mneil commented Nov 17, 2023

My opinion is that the library should not make any assumptions about how it will be consumed. Users choosing to bundle with webpack need to understand the implications of choosing that tool and how to work with it. Rollup, Turbopack, esbuild, Webpack, and other tools exist to do the same thing. I'm not sure pyodide should attempt to do anything specifically for one or more of these tools.

The plugin now has a custom loader that skips parsing by webpack so that it doesn't import anything. It's close to adding the ignore comments on the imports. It also handles copying the necessary dependencies like pyodide.asm.js and other files. Obviously I'm biased in favor of the plugin :)

@hoodmane
Copy link
Member

hoodmane commented Nov 17, 2023

close to adding the ignore comments on the imports

Meaning it doesn't do this yet? So maybe we should leave them in for now and then drop all of these once you add support for this?

@mneil
Copy link
Contributor

mneil commented Nov 17, 2023

No, I'm sorry for the confusion. What I meant by "close to adding" is that the functionality is similar, but it is accomplished in a different way. It does not need to add those comments because it doesn't go through webpack's javascript parser anymore.

The comments are not necessary for the plugin to work correctly.

@hoodmane
Copy link
Member

hoodmane commented Nov 17, 2023

Ok then I vote to drop these because they are a wart. If we want to improve webpack-specific support, we should focus on improving the discoverability of the plugin.

@ryanking13
Copy link
Member Author

Thanks for the info! I agree that the handling of bundler-specific things should be handled in the plugin. Maybe let's include this to 0.25.0 for now, and find a way to improve the discoverability of the plugin + test the plugin in the Pyodide repository.

@hoodmane
Copy link
Member

hoodmane commented Jan 4, 2024

include this to 0.25.0 for now, and find a way to improve the discoverability of the plugin + test the plugin in the Pyodide repository

Should we open one or more followup issues for these tasks?

@ryanking13
Copy link
Member Author

I opened a followup issue (#4361)

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.

Cannot find module 'https://cdn.jsdelivr.net/pyodide/dev/full/pyodide.asm.js'
3 participants