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

Option to choose list of files to be copied #12

Open
ryanking13 opened this issue Mar 1, 2023 · 12 comments
Open

Option to choose list of files to be copied #12

ryanking13 opened this issue Mar 1, 2023 · 12 comments

Comments

@ryanking13
Copy link
Member

First of all, thanks for maintaining this project Michael, I really appreciate it.

As mentioned in pyodide/pyodide-webpack-example#7, it would be nice to test this plugin in Pyodide repository and make sure the upstream changes don't break this plugin. For that, I think we need some way to override the list of files to be copied, as upstream changes may add/remove files to be copied.

Maybe something like:

module.exports = {
  plugins: [new PyodidePlugin(
    files: ["pyodide.js", "pyodide.asm.js", "pyodide.asm.wasm"],
  )],
};
@ondrej-stanek-ozobot
Copy link

+1 here, great idea! We started using this neat plugin recently. Because our pyodide build has some additional files included in the npm package, we need to patch the lib/patterns.ts source file with additional files. This improvement to the plugin would allow to avoid the patching altogether.

@ondrej-stanek-ozobot
Copy link

I would like to propose an alternative to specifying the file list in constructor. Please consider the following:

The list of files that constitute each pyodide build is already listed in the pyodide's package.json file. The pyodide-webpack-plugin already locates and parses the pyodide's package.json file in the tryResolvePyodidePackage function. So perhaps the plugin itself could consult the package.json file of the particular pyodide's build that is to be served, and based on the contents of the files section it would build a list of arguments for the CopyPlugin.

@mneil , what is your opinion on this? Are there any risks or alternate usage scenarios for which this automatic file list retrieval would not work?

@mneil
Copy link
Collaborator

mneil commented Mar 1, 2023

@ondrej-stanek-ozobot I do like that idea. This was not the case when the plugin was originally developed. But we could keep the internal list for older versions. Going forward we can rely on the files in the package.json. The one issue I see with that is the files list contains extra data like types definitions and html.

Maybe we could do a combination approach? Copy over the list of files in the "files" property of package.json, exclude html and d.ts files, but also allow an override as Gyeongjae mentioned as an override.

@ryanking13 what do you think about these suggestions?

@hoodmane
Copy link
Member

hoodmane commented Mar 1, 2023

Is copying the html / d.ts files harmful or simply unnecessary?

@mneil
Copy link
Collaborator

mneil commented Mar 1, 2023

It is unnecessary and these files would go unused.

@hoodmane
Copy link
Member

hoodmane commented Mar 1, 2023

Presumably there are other unnecessary files in the files list, but there isn't a per-file-extension rule? e.g., what about pyodide.js and pyodide.mjs? Isn't pyodide.js getting webpacked into the main bundle?

@mneil
Copy link
Collaborator

mneil commented Mar 1, 2023

Yes, pyodide.js (or pyodide.mjs) are getting put into the main bundle. The plugin does not currently copy either of those over.

@hoodmane
Copy link
Member

hoodmane commented Mar 1, 2023

But if we switch to using the files list then they will get copied? Sorry for my dumb questions. I like this files idea and I think the point is that we only need an overestimate of which files are needed.

@mneil
Copy link
Collaborator

mneil commented Mar 1, 2023

Yes, then they would both get copied. These files are copied into the package the user intends to distribute or put into production. There would be a size increase to that output with no benefit to the user.

@hoodmane
Copy link
Member

hoodmane commented Mar 1, 2023

size increase to that output

But these extra files don't get sent to the client browser? So it's not desirable but also not a huge problem?

@mneil
Copy link
Collaborator

mneil commented Mar 1, 2023

No, they would not get sent to the browser.

@ryanking13
Copy link
Member Author

@ryanking13 what do you think about these suggestions?

Great! I like the combination approach.

mneil added a commit that referenced this issue Sep 22, 2023
Addresses part of #12 by guessing which files are needed to
go into the bundle based on pyodide package.json files list. Excludes
some files like d.ts and .html files. Ensure certain files like
package.json are always copied.
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

No branches or pull requests

4 participants