vite icon indicating copy to clipboard operation
vite copied to clipboard

feat: support preloading also on legacy bundle

Open Tal500 opened this issue 1 year ago • 6 comments

Description

Support preloading also on legacy bundle.

A test case is included for a utility function I'm using.

closes #9902 and even more (since it's also preload JS scripts) fixes #2062 closes #5901 fixes #9967 (by a more careful checking on the preload function)

Additional context

As mentioned in #9902, by solving the issue with this PR, it makes the legacy build support for SvelteKit I'm working on(sveltejs/kit#6265) to load correctly the CSS files when navigating(and more correctly, on preloading) from one page to the other.


What is the purpose of this pull request?

  • [X] New Feature
  • [ ] Documentation update
  • [ ] Other

Before submitting the PR, please make sure you do the following

  • [X] Read the Contributing Guidelines.
  • [X] Read the Pull Request Guidelines and follow the Commit Convention.
  • [X] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • [ ] Ideally, include relevant tests that fail without this PR but pass with it.

Tal500 avatar Aug 30 '22 21:08 Tal500

Help: Don't know why vue legacy tests are failing.

Tal500 avatar Sep 01 '22 09:09 Tal500

It seems this image is not being loaded when doing pnpm build && pnpm preview in playground/vue-legacy. https://github.com/vitejs/vite/blob/4158b98c8fc30070967de1176f4b1c252d275386/playground/vue-legacy/module.vue#L9

sapphi-red avatar Sep 02 '22 12:09 sapphi-red

It seems this image is not being loaded when doing pnpm build && pnpm preview in playground/vue-legacy.

https://github.com/vitejs/vite/blob/4158b98c8fc30070967de1176f4b1c252d275386/playground/vue-legacy/module.vue#L9

Tnx! I had realized that the root cause for the failure is because I'm fighting with Vite way to inline the CSS on legacy. But the immediate question now is, why? I have commented this in the original issue I had opened: https://github.com/vitejs/vite/issues/9902#issuecomment-1236194069

Tal500 avatar Sep 03 '22 20:09 Tal500

I tested with Chrome 16 and it worked greatly. 👍 (when minify is disabled)

sapphi-red avatar Sep 05 '22 08:09 sapphi-red

Test now, it's working on my machine also after minification, including testing.

Note: As said, I had stripped out the XHR call. This results with the side effect that if the network resource(the CSS file) isn't available, then no exception will be raised on legacy.

If this is crucial I can return this extra call.

Note2: I had modified a little bit the directory structure of the testing, to include the code samples. Hope it's fine.

Tal500 avatar Sep 05 '22 13:09 Tal500

Now I think it's ready. ~Suggest to merge #9970 first, both for credits and test units.~

Tal500 avatar Sep 10 '22 18:09 Tal500

I had removed the fix of #9967 from this PR, since it's handled better in #9970. Now both #9970 and this PR are independent, and merging one won't affect the other.

Note: Need to still implement in the future the suggestion appears in the discussion of https://github.com/vitejs/vite/pull/9970#discussion_r969505865, but this can be done later in a different PR, as this suggestion is nice to have.

So now I'm ready for review&merge.

Tal500 avatar Sep 18 '22 12:09 Tal500

This implementation depends on regexs very heavily and I felt a bit fragile. So I have a suggestion, how about using instantiate hook of SystemJS? In this way, I think we could avoid regexs.

In entry chunks:

if (!window.__viteLegacyDeps) {
  window.__viteLegacyDeps = {}

  const originalInstantiate = System.constructor.prototype.instantiate
  System.constructor.prototype.instantiate = function (args) {
    // preload using the information of `window.__viteLegacyDeps`
    return existingHook.call(this, args)
  }
}

In each chunks:

window.__viteLegacyDeps['chunk file'] = [/* dep list */]

sapphi-red avatar Oct 17 '22 05:10 sapphi-red

This implementation depends on regexs very heavily and I felt a bit fragile.

This is why I added testing snapshots😃 I agree however about the principle. ESModules import are scanned well by Vite. Do you think we can somehow generate a map at the transform/renderChunk stage(or such) between the es-formatted chunk to their (well-)detected dynamically imported (other) chunks, and then use it directly for writing the dependency list?

In each chunks:

window.__viteLegacyDeps['chunk file'] = [/* dep list */]

How do we get the dependency list here? Feels like we need to have the regexps (or an alternative solution) for this again.

Tal500 avatar Oct 17 '22 19:10 Tal500

Do you think we can somehow generate a map at the transform/renderChunk stage(or such) between the es-formatted chunk to their (well-)detected dynamically imported (other) chunks, and then use it directly for writing the dependency list?

I think we can't. Dependency graph can only be obtained in renderChunk/generateBundle, but in those hooks the code is already transformed to SystemJS.

How do we get the dependency list here? Feels like we need to have the regexps (or an alternative solution) for this again.

I guess we can use dynamicImports property of ChunkInfo to get what is dynamic-imported. Tracing static-imports could be done like we do now. https://rollupjs.org/guide/en/#generatebundle

sapphi-red avatar Oct 18 '22 09:10 sapphi-red

Do you think we can somehow generate a map at the transform/renderChunk stage(or such) between the es-formatted chunk to their (well-)detected dynamically imported (other) chunks, and then use it directly for writing the dependency list?

I think we can't. Dependency graph can only be obtained in renderChunk/generateBundle, but in those hooks the code is already transformed to SystemJS.

How do we get the dependency list here? Feels like we need to have the regexps (or an alternative solution) for this again.

I guess we can use dynamicImports property of ChunkInfo to get what is dynamic-imported. Tracing static-imports could be done like we do now. https://rollupjs.org/guide/en/#generatebundle

I deepen into Rollup chunk information, but it seems to tell us only about the dependencies sources, but not on the location on the chunk that the "import" is used, and parseImports/parseImportsSystemJS give us this extra information.

The parseImports is used twice:

  1. On the transform stage.
  2. On the generateBundle phase.

On the first one, the tailor-made new function parseImportsSystemJS isn't needed, since on the transform stage, the code is always in the es format, before the legacy conversions, so no problem at all.

On the second one however, the format might be es(modern) or system(legacy), so this is why we have both parseImports and parseImportsSystemJS, where the latter uses regular expressions.

Now return to what I started with in this comment - it seems that we need the locations of the dynamic imports on the generateBundle stage only for one good reason - to remove the pure CSS chunk imports: https://github.com/vitejs/vite/blob/4b8a58703055a0eb5d8ddb04ef9f1eb081fbbc6a/packages/vite/src/node/plugins/importAnalysisBuild.ts#L525-L540

If we can overcome this issue, we can eliminate the need of the function parseImportsSystemJS.

Additionally, another sad truth - we still need the newly introduced function analyzeSystemRegistration, since it used (also!) for removing pure css chunks in the css plugin: https://github.com/vitejs/vite/blob/6d4d82e87320c3361b41e8d831731bd6a0d57ed7/packages/vite/src/node/plugins/css.ts#L630-L655

How then can we overcome the removing process of pure css imports?

Tal500 avatar Oct 19 '22 08:10 Tal500