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

feat: support preloading also on legacy bundle #9920

Open
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

Tal500
Copy link
Contributor

@Tal500 Tal500 commented Aug 30, 2022

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 #10285

Additional context

As mentioned in #9902 and #10285, 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?

  • New Feature
  • Documentation update
  • Other

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

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • 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
Copy link
Contributor Author

Tal500 commented Sep 1, 2022

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

@sapphi-red
Copy link
Member

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

background: url('@/assets/asset.png') no-repeat;

@Tal500
Copy link
Contributor Author

Tal500 commented Sep 3, 2022

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

background: url('@/assets/asset.png') no-repeat;

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:
#9902 (comment)

@sapphi-red
Copy link
Member

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 */]

@Tal500
Copy link
Contributor Author

Tal500 commented Oct 17, 2022

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.

@sapphi-red
Copy link
Member

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

@Tal500
Copy link
Contributor Author

Tal500 commented Oct 19, 2022

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:

const removedPureCssFiles =
removedPureCssFilesCache.get(config)!
const chunk = removedPureCssFiles.get(filename)
if (chunk) {
if (chunk.viteMetadata.importedCss.size) {
chunk.viteMetadata.importedCss.forEach((file) => {
deps.add(file)
})
hasRemovedPureCssChunk = true
}
s.overwrite(expStart, expEnd, 'Promise.resolve({})', {
contentOnly: true
})
}
}

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:

const importExp = (() => {
switch (opts.format) {
case 'es':
return new RegExp(
`\\bimport\\s*["'][^"']*(?:${emptyChunkFiles})["'];\n?`,
'g'
)
case 'system':
return (code: string) => {
const moduleParam = analyzeSystemRegistration(code)?.moduleParam
return moduleParam !== undefined
? new RegExp(
`\\b${moduleParam}.import\\s*["'][^"']*(?:${emptyChunkFiles})["'];\n?`,
'g'
)
: null
}
case 'cjs':
return new RegExp(
`\\brequire\\(\\s*["'][^"']*(?:${emptyChunkFiles})["']\\);\n?`,
'g'
)
default:
return null
}
})()

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

@Tal500
Copy link
Contributor Author

Tal500 commented Nov 9, 2022

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, continuing the logic in my previous comment, do you think that any (breaking?) change of Vite 4.0 can help us here?

@sapphi-red
Copy link
Member

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.

We only need that what will be dynamically imported in that chunk. I'm suggesting to declare the following code in the head of the chunk, not aside of the import.

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

For example, transform this code

// main.000.js ------------------------------
System.register([], function (exports, module) {
  module.import('foo.111.js')
  module.import('bar.222.js')
});

// foo.111.js ------------------------------
System.register([], function (exports, module) {
  console.log('foo.111.js')
});

// foo.111.css ------------------------------
/* css content */

// bar.222.js ------------------------------
System.register([], function (exports, module) {
  console.log('bar.222.js')
  module.import('foobar.333.js')
});

// foobar.333.js ------------------------------
System.register([], function (exports, module) {
  console.log('fioobar.333.js')
});

to

// main.000.js ------------------------------
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)
  }
}

window.__viteLegacyDeps['foo.111.js'] = ['foo.111.css']
window.__viteLegacyDeps['bar.222.js'] = []

System.register([], function (exports, module) {
  module.import('foo.111.js')
  module.import('bar.222.js')
});

// foo.111.js ------------------------------
System.register([], function (exports, module) {
  console.log('foo.111.js')
});

// foo.111.css ------------------------------
/* css content */

// bar.222.js ------------------------------
window.__viteLegacyDeps['foobar.333.js'] = []

System.register([], function (exports, module) {
  console.log('bar.222.js')
  module.import('foobar.333.js')
});

// foobar.333.js ------------------------------
System.register([], function (exports, module) {
  console.log('fioobar.333.js')
});

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

How about injecting window.__viteLegacyPureCss = [/* pure css deps */] and skipping them in instantiate when preloading?

continuing the logic in my previous comment, do you think that any (breaking?) change of Vite 4.0 can help us here?

I'm not able to track the changes deeply but I guess no.

@Tal500
Copy link
Contributor Author

Tal500 commented Nov 14, 2022

Took some work but finally made it with SystemJS hooks!
Ready for review @sapphi-red :-)

@sapphi-red
Copy link
Member

sapphi-red commented Jun 22, 2023

I'm sorry that it took a long time.

I tried this PR out and found that it doesn't work when there's a dynamic import in a dynamic imported chunk.
For example, in 211f4a7, "async2" should be blue but it isn't in legacy.
I guess this is because this PR's implementation only injects a single __VITE_PRELOAD_LIST__. __VITE_PRELOAD_LIST__ has to exist in every chunk.

@Tal500
Copy link
Contributor Author

Tal500 commented Jun 22, 2023

I'm sorry that it took a long time.

I tried this PR out and found that it doesn't work when there's a dynamic import in a dynamic imported chunk. For example, in 211f4a7, "async2" should be blue but it isn't in legacy. I guess this is because this PR's implementation only injects a single __VITE_PRELOAD_LIST__. __VITE_PRELOAD_LIST__ has to exist in every chunk.

What is the way to go with that?
I also wasn't sure about the global declaration policy in Vite, and the chunk loading.

@sapphi-red
Copy link
Member

What is the way to go with that?

I think injecting the following code to every chunk in generateBundle hook would work.

${preloadListMarker}.forEach((data) => {
  ${preloadLegacyImportsToken}[assetsURL(data[0], import.meta.url)] = {
    deps: data[1],
    pure: data.length === 3
  }
})

I also wasn't sure about the global declaration policy in Vite, and the chunk loading.

This PR already depends on window.__viteLegacyImports. What do you mean by "global declaration policy"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority) plugin: legacy
Projects
Status: Approved
3 participants