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

Can't import CJS deep import #1438

Closed
jods4 opened this issue Jan 8, 2021 · 9 comments
Closed

Can't import CJS deep import #1438

jods4 opened this issue Jan 8, 2021 · 9 comments

Comments

@jods4
Copy link
Contributor

jods4 commented Jan 8, 2021

This is similar to many deep import bugs, which are closed.
There's currently #1041 that is open, but it seems the cause there is linking.
This looks exactly like #1067, which was closed because its author was "lucky" having access to ESM sources.

Describe the bug

I'm trying to use prismjs and include the csharp language grammar.
Here's my module:

import Prism from "prismjs";
export default Prism;
await import("prismjs/components/prism-csharp");

Note that prismjs is entirely written in Common JS style (and even worse: uses global variables, hence the await import()).

When I try to import this module, Vite says this on the console:

22:22:09 [vite] Internal server error:
  Plugin: vite:imports
  File: /App/prism.ts
  Error: Deep import "prismjs/components/prism-csharp" should be avoided because dependency "prismjs" has been pre-optimized. Prefer importing directly from the module entry: 

  import { ... } from "prismjs"

  If the used import is not exported from the package's main entry and can only be attained via deep import, you can explicitly add the deep import path to 
"optimizeDeps.include" in vite.config.js.

So I followed the instruction and added optimizeDeps: { include: [ "prismjs/components/prism-csharp"] } to my config, which changed nothing.

I tried anything that came to mind, like adding .js to the path, trying exclude: ["prismjs"], nothing worked.

From the comments in #1067 I have the impression that you can't exclude this lib from pre-opt because it's written in CommonJS style.
That issue seem to be the same bug but it was closed without a fix (workaround was to load a ESM source instead... when available).

System Info

  • vite version: 2.0.0-beta.12
  • Operating System: Win 10
  • Node version: 15
  • Package manager and version: npm 7
@yyx990803
Copy link
Member

It works. You'll need to provide a repro.

@jods4
Copy link
Contributor Author

jods4 commented Jan 8, 2021

@yyx990803 Not sure what repro to produce, those 3 lines are literally the entire module they're in.
Prismjs is available on npm.
If that makes any difference, I use the vite JS API and pass the config directly to it.

@yyx990803
Copy link
Member

yyx990803 commented Jan 8, 2021

I already spent 10 minutes reproducing this and Prism is correctly available on window with Prism.lanaguages.csharp present. The top level await confuses me, since no browser supports it natively yet. Which is why a repro is necessary. Please provide one when asked, there's no point going back and forth on this.

@jods4
Copy link
Contributor Author

jods4 commented Jan 9, 2021

OK, while building the repro I noticed: it works when using the CLI but not when passing options to the JS API.

You can clone this repro, it is vite vue-ts template with prism added as in the issue description:
https://github.com/jods4/vite-1438

If you run npm run dev it uses the CLI and won't work because I commented the include in vite.config.js.
If you uncomment it, it would work.
If you run npm run js-api then it uses the API and passes the include in an inline config object. That doesn't work, unless I'm using the API incorrectly?

The top level await confuses me, since no browser supports it natively yet.

Supported in Node 14.3+ and Chrome/Edge 89+.
Firefox as well, but behind a setting.

BTW is there a better way to import Prism without relying on await import?
The problem is that the second file needs a global Prism object already initialized.
Non-module <script> are synchronous so that's an option, but you can't rely on ordering with static import. Code is imported and run in no particular order, so if the .csharp extension comes faster than prismjs then it will run before it and fail.

@yyx990803
Copy link
Member

As of e438802 the following should just work without any config:

import Prism from 'prismjs'
import 'prismjs/components/prism-csharp'

export default Prism

No need for top level await.

@jods4
Copy link
Contributor Author

jods4 commented Jan 29, 2021

@yyx990803

No need for top level await

Can you please explain to me what does Vite do to guarantee that it works?

I thought Vite served files as native modules but the ESM specs (to the best of my knowledge) doesn't mandate an order of execution identical to source code imports. It only requires them to have all been initialized before the module is actually run.

The problem with prismjs/components/prism-csharp is that it doesn't import prismjs. It assumes that a global var is already available. This was all fine in the pre-module <script> era, or in Node with require, which both guarantee blocking, ordered semantics.

With ESM no dependency exist between these two modules, so prismjs/components/prism-csharp would be free to run before prismjs if the file is served faster by the web server. And then the global var doesn't exist yet and you have a race condition to debug.

@jods4
Copy link
Contributor Author

jods4 commented Jan 29, 2021

I'm gonna add that I'm aware that both Webpack and Rollup offer ordering guarantee when possible (non-circular, etc.) inside a single chunk (no code-splitting, etc.). So this code is fine when bundled.

Since Vite is serving file as unmodified ESM modules to browsers, I'm just curious how it works during dev.

@yyx990803
Copy link
Member

yyx990803 commented Jan 29, 2021

I tested in all major browsers and the de facto behavior is that they all respect import statement order in terms of evaluation. The modules are fetched in parallel, but the code evaluation are strictly performed in import order. The spec itself isn't super clear about this, but that is the behavior that all major browsers have decided to align with (which makes sense IMO, since there isn't major perf loss in this - parse and fetch can be performed without actually evaluating the module).

The code works even if I artificially delay the response for the main prism.js file by a second.

@jods4
Copy link
Contributor Author

jods4 commented Jan 30, 2021

That surprised me, so I did more research. I was wrong (but caveats apply, see below).

which makes sense IMO, since there isn't major perf loss in this - parse and fetch can be performed without actually evaluating the module

Quick aside: actually, there is perf to win here. You could execute modules that are already ready in parallel with parsing/fetching of slower/larger previous modules, hence completing earlier.

First I did some tests (Win, Edge 89).
Basic test setup:

// main.js
import './a'
import './b'
console.log("Main")

// a.js
console.log('A')

// b.js
console.log('B')

Basic run is A -> B -> Main.

Then I blocked script A using Fiddler. 10s later nothing happened. When I released the module in Fiddler everything ran A -> B -> Main, so clearly the browser was waiting for in-order execution.

De facto standard is a dangerous thing is it may change based on heuristics, platforms, new releases... So I checked the EcmaScript specification and turns out in-order execution might actually be specified in there.

In §15.2.1.16 it defines [[RequestedModules]] as "A List of all the ModuleSpecifier strings used by the module represented by this record to request the importation of a module. The List is source code occurrence ordered." (emphasis mine, the requested modules are source code ordered.)

Then §15.2.1.16.5 ModuleEvaluation() Concrete Method says:

6. For each String required that is an element of module.[[RequestedModules]] do
[...]
  [6.]c. Let status be requiredModule.ModuleEvaluation().

So that implies evaluation of imported modules happen in source order. ✔️

Of course, that's assuming modules A and B are independent, otherwise module A would trigger evaluation of module B itself, but that's really what you'd expect.

Except I found a caveat where statically importing A, B is different from importing A then dynamically importing B.
I made module A async:

await new Promise(r => setTimeout(r, 5000))
console.log('A')

And guess what? Module B evaluation started before A completed (of course Main waited for A to complete).
So the outcome was B -> A -> Main.

Other than that, I suppose one can assume that independent, synchronous modules evaluate in order, which is nice for modules that have global effects, notably importing polyfills at the start of your program.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants