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

Parcel, Rollup JS bundlers & Next.js(solved) have compatibility issues with Pyodide npm package (0.18.2) #1949

Open
grimmer0125 opened this issue Nov 12, 2021 · 13 comments

Comments

@grimmer0125
Copy link
Contributor

grimmer0125 commented Nov 12, 2021

They have the same issues as Webpack bundler, introduced in #1875 and #1900.

They seem to be famous bundlers, except Webpack. I have not really used them before, and I notice these Pyodide npm package compatibility issues when I try to bundle my another side project numjs. Rollup may be OK since it may not be widely used in front-end developers like Parcel. For Parcel, I do not exactly know how many people use it. In my limited experience about this time experience on my project, it is a good tool.

I do not think this should definitely count as Pyodide issue, and they can count as Parcel/Rollup side issues. I open this issue is to leave a note for people.

@grimmer0125 grimmer0125 added bug Something isn't working documentation and removed bug Something isn't working labels Nov 12, 2021
@rth
Copy link
Member

rth commented Nov 12, 2021

Thanks for checking this @grimmer0125 ! For rollup it's a bit surprising since we are using it internally as a bundler and it works OK there; maybe something is wrong in the way we setup the npm package.

@grimmer0125
Copy link
Contributor Author

grimmer0125 commented Nov 13, 2021

I've tested the other two front-end tools, Vite (OK) and Next.js(Fail).

Vite (Rollup base):

  • Use yarn create vite vite-react-app --template react to generate a test project.
  • Then it runs out of the box. It has a warning but can be eliminated by await import(/* @vite-ignore */path.resolve(url));
warning: 
/Users/grimmer/git/vite-react-app/node_modules/.vite/pyodide_pyodide_js.js
110|      } else {
111|        const path = await pathPromise;
112|        await import(path.resolve(url));
   |                     ^
113|      }
114|    };
The above dynamic import cannot be analyzed by vite.

Next.js (Webpack base):

  • Generate the test project: npx create-next-app nextjs-blog --use-npm --example "https://github.com/vercel/next-learn/tree/master/basics/learn-starter"
  • Modify the code to let it build successfully.
async function loadPyodide() {
  const pyodide_pkg = await import("pyodide/pyodide.js");
  const pyodide = await pyodide_pkg.loadPyodide({
    indexURL: "https://cdn.jsdelivr.net/pyodide/v0.18.1/full/",
  });

  pyodide.runPython(`
    import js
    div = js.document.createElement("div")
    div.innerHTML = "<h1>Hello Pyodide2! This element was created from Python</h1>"
    js.document.body.prepend(div)
  `);
}

export async function getStaticProps() {
  // running int server side 
  await loadPyodide();
  return;
}

export default function Home() {
  useEffect(() => {
    // runPyodide();
  }, [])

截圖 2021-11-13 下午3 07 20

1123 update:
if putting the code in useEffect (client side), got the below runtime error: Module.runPythonInternal is not a function,
截圖 2021-11-23 下午2 37 00
commenting these codes will run sucessfully.

Module.runPythonInternal(`
    import importlib
    importlib.invalidate_caches();
  `);

--

For front-end developing, sum up:

  • Top popular:
    • Vanilla JS
    • Most knwon bundler: Webpack
    • Popular Front-end tool: CRA (create-react-app), *Nest.js (React+Server Side Rendering), Angular CLI, Vite
  • Secondary popular: Parcel, Rollup <- they are made for generic purpose including Node.js, not front-end specified.

I would say supporting those top popular ones might be enough. Nest.js is widely used by React developers and supporting it should be a big plus. But according to my experience, some npm packages fail to run on it or need to figure out how to run on it (e.g. add code to detect window/node environment), this is due to its SSR (Server Sider Rendering) design and make things much complicated. For study purposes, configing next.js is interesting but it is annoying from a user point of view, the drawback of next.js.

Maybe packaging everything as multiple npm packages (like #1477 ), e.g. @pyodide/core, @pyodide/numpy will let installation easier, for an end-user.

@grimmer0125 grimmer0125 changed the title Parcel and Rollup JS bundlers have compatibility issues with Pyodide npm package (0.18.2) Parcel, Rollup JS bundlers & Next.js have compatibility issues with Pyodide npm package (0.18.2) Nov 13, 2021
@hoodmane
Copy link
Member

hoodmane commented Nov 13, 2021

Rollup: Test rollup/rollup-starter-app and encounter the below error,

I think the solution to this is provided in the docs
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
which is to add the following to rollup.config.js:

  external: ['fs/promises'] // <-- suppresses the warning

Or does that not help?

@grimmer0125
Copy link
Contributor Author

grimmer0125 commented Nov 14, 2021

Or does that not help?

Yes and No. Actually, there are 3 errors and I do not paste two of them in the first comment. After setup external: ['fs/promises'] and the related error is fixed but the other two still exist.

The original 3 errors (2 yellow and 1 red)
截圖 2021-11-14 下午9 30 42

@Louis-Tian
Copy link

Can we leverage on the conditional exports feature that is available from node 12+ for a more universal solution to this problem?
https://nodejs.org/api/packages.html#conditional-exports
This feature allows us to define multiple entry points to a packages depends on the execution environment.

Fix in #1900 is webpack specific, I think this would be nicer solution?

Related to this, webpack used to polyfill Node's core modules out of the box, which is no longer the case in webpack 5. And the recommendation from webpack for migrating from webpack 4 to 5 h reads:

  • webpack 5 does no longer include a polyfill for this Node.js variable. Avoid using it in the frontend code.
  • Want to support browser usage? Use the exports or imports package.json field to use different code depending on the environment.

@grimmer0125
Copy link
Contributor Author

grimmer0125 commented Nov 23, 2021

@Louis-Tian thank you for the information. I'm trying to make a summary of the issues below.

  • Node.js API (e.g. file) issue, since Pyoidie is trying to support Node.js usage: happens in Rollup/Webpack(fixed) (which is based on Webpack but obviously it has customized Webpack). solution:
    • one output: polyfill/mock/better dynamic skip code (but it is hard since bundler is running in Node.js env when building/bunldering time and they may throw exceptions at this time)
      • improvement on Pyodide side (e.g. put webpack ignore flag) / Bundler side (smart enough to auto-detect)
      • or write documents to tell each user how to customize their bundlers
    • two output entries (possibly need two versions of source code for browser/node.js) in 1 npm package. In package.json, we possibly can use
  • Browser issue: dynamic import for http fetching Pyodide WebAssembly files: happens in Parcel(Wait Allow ESM imports from absolute urls parcel-bundler/parcel#4148 fix)/Webpack(fixed) runtime, and not sure whether Rollup will happen or not. solution:
    • in the long-term, personally, I think publishing everything as @pyodide/core, @pyodide/numpy will reduce this two-step importing issue.
    • figure out how to customize bundlers or find out possible bundler flags on Pyodide side
  • Uknown issue: when running Next.js app, got Module.runPythonInternal is not a function error and commenting that part code let Pyodide work.

I updated #1949 (comment) to include Next.js above runtime case and the original case/error is running in its server-side and it's a corner case, we can ignore it.

Summary: we can figure out the remaining issues (in a short term)

  • Parcel: wait for its fix, or merge the dynamic import JS part of loadPyodide/loadScript to one Pyodide npm package.
  • Rollup.js - need to figure it out
  • Next.js (using CDN Pyodide is working) Module.runPythonInternal is not a function : The cause is that I copy the load-pyodide.js from Pyodide main branch to overwrite the one in Pyodide npm package. And it will happen not only in next.js but also Webpack test case. The change diff is #bb75150. @hoodmane is it a bug or it needs to work with the latest built Pyodide from the main branch?

ref:

@grimmer0125 grimmer0125 changed the title Parcel, Rollup JS bundlers & Next.js have compatibility issues with Pyodide npm package (0.18.2) Parcel, Rollup JS bundlers & Next.js(solved) have compatibility issues with Pyodide npm package (0.18.2) Nov 23, 2021
@Louis-Tian
Copy link

Louis-Tian commented Nov 23, 2021

Hi @grimmer0125

one output: polyfill/mock/better dynamic skip code (but it is hard since bundler is running in Node.js env when building/bundlering time and they may throw exceptions at this time)

Just to clarify, I think the problem is not that webpack or parcel are Node.js based.

The problem lies with the way how those bundlers handle imports.

An import can be either dynamic await import(foo) or regular import 'fs/promise'.
When a bundler's parser encounters an import statement, it will try to locate the module and included it in the bundle.
This means it will

  • inline the content of the module being imported in case of regular import,
  • create a seperate entrypoint in case of dynamic import (this is related the code splitting feature)

Please note those parsers are static, they have no understanding of any runtime context.

The problem rises when the build target is set to an non-nodejs environment. (i.e. web). In these cases, the parser reads lines like await import('fs/promise') and tries to include those nodejs' core modules into the bundle. On the other hand, if we set the build target to 'node', the bundler is made aware of those special modules and will skip over the line when importing a node builtin modules.

On top of the above. we also have an issue that is specific to dynamic import on this line

loadScript = async (url) => await import(/* webpackIgnore: true */ url);

As noted in webpack documentation here. a full dynamic import is not possible. without the /* webpackIgnore: true */ you applied in #1900, this will throw error as well. This is significant because it means we need a special flag for every single bundling tool that we support and it simply won't work for tools without such flag (say Parcel as it is now). I agree with your comment on merge into the core package. Right now the loadScript function is used to import the pyodide.asm.js package at runtime, which is unnecessary, as they can be combined at build time, which completely eliminates this dynamic import problem.

@Louis-Tian
Copy link

Louis-Tian commented Nov 24, 2021

Further to the thoughts above. Given that all of the offending lines are contained within the loadScript function, whose sole function is to load the pyodide.asm.js into the global scope for side effects. Why can't we simply import it with a regular import statement. something like import '../build/pyodide.asm.js'?


okay, it's not just the loadScript function, there is also initializePackageIndex and loadPackage functions. But the similar kind import could be used for those as well I think.

@grimmer0125
Copy link
Contributor Author

grimmer0125 commented Nov 26, 2021

Hi,

I have roughly figured out the needed modification to use Pyodide npm on rollup-starter-app. I put the change here, grimmer0125/pyodide-rollup-starter-app#1.
Three main issues :

  1. [node.js polyfill] fs/promise external setting
  2. [node.js polyfill] need to install @rollup/plugin-json for tr64 used by node-fetch. ref: It is not bundled with Rollup jsdom/tr46#28
pyodide
  node-fetch  
    whatwg-url@^5.0.0:
      tr46
  1. default iife output fails, need to use es, otherwise the below error shows up. To be honest, I do not know why it uses code-splitting + iife by default and why it fails.
[!] Error: Invalid value "iife" for option "output.format" - UMD and IIFE output formats are not supported for code-splitting builds.

@FossPrime
Copy link

FossPrime commented Nov 8, 2022

Support seems to be perfectly fine... issue should probably be closed

@hoodmane
Copy link
Member

hoodmane commented Nov 8, 2022

Thanks for looking into this @FossPrime. @mneil helped and fixed our package.json so that it includes all of our files and exports:
https://github.com/pyodide/pyodide/blob/main/src/js/package.json#L49-L74
I still have some questions:

  1. Did vite look at the list of exports to decide what to bundle? Or the list of files? Or what?
  2. What files did vite bundle?

To work, Pyodide needs at least the following files:

  • pyodide.asm.js
  • pyodide.asm.wasm
  • pyodide.asm.data
  • pyodide_py.tar
  • pyodide.mjs or pyodide.js
  • repodata.json

We don't need console.html, pyodide.d.ts, or pyodide.(m)js.map. Probably most bundlers are smart enough not to bundle typescript definitions or source maps. If it added console.html that's only 7.3kb. pyodide.d.ts is 25kb and pyodide.js.map is 78kb. So even if it did include all these, it would increase the size of the bundle by less than 2%.

Note also that webpack still doesn't work without using @mneil's pyodide-webpack-plugin.

So we've made a lot of progress on this but there is still probably work to be done here?

@TheLostLambda
Copy link

Support seems to be perfectly fine... issue should probably be closed

Is there any way to bundle Pyodide without needing to set the indexURL to the CDN? That's not even a solution that works for me because of #3550.

I'm currently stuck loading things from a CDN and have had to give up on the NPM package entirely — perhaps things have broken or maybe I'm just being silly?

Sorry for pestering and thanks for your help!

@TatyanaVl
Copy link

I have the same request as @TheLostLambda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants