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

fix: restore dynamic-import-polyfill #3434

Merged
merged 5 commits into from
May 17, 2021
Merged

fix: restore dynamic-import-polyfill #3434

merged 5 commits into from
May 17, 2021

Conversation

patak-dev
Copy link
Member

Description

This PR restores the dynamic import polyfill plugin that was removed in #2976 with build.polyfillDynamicImport disabled by default.

@vitejs/legacy-plugin will use this plugin setting build.polyfillDynamicImport to true.

There are two changes compared to the old plugin code: https://github.com/vitejs/vite/blob/02ba4ba32cd40f1cc3943781022c05f9df3b57e6/packages/vite/src/node/plugins/dynamicImportPolyfill.ts

  1. The plugin is always included so we can use it to issue a warning when the user imports vite/dynamic-import-polyfill but the polyfill feature is disabled.
  2. Computing the polyfill string has now been moved inside load, as with the new default it is no generally needed. This move also replaced the need for the extra boolean loaded flag.

What is the purpose of this pull request?

  • Bug fix
  • 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.

@patak-dev patak-dev requested a review from antfu May 15, 2021 12:44
@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label May 15, 2021
self[importFunctionName] = new Function('u', `return import(u)`)
} catch (error) {
const baseURL = new URL(modulePath, location)
const cleanup = (script: any) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Is script an HTMLScriptElement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the exact code from the polyfill from google that was removed, so I would prefer to avoid touching it. If we want to improve it, we can do so in another PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTMLScriptElement would be just only a compile time code addition
Would not touch runtime code 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss it in another PR 👍🏼

// if dynamic import polyfill is used, rewrite the import to
// use the polyfilled function.
if (isPolyfillEnabled) {
s.overwrite(dynamicIndex, dynamicIndex + 6, `__import__`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: What is 6? It's a magic number in this context 🤔
Should we extract it into a variable/constant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 is the number of letters in import, this is the same code that was removed before so it is ok to add it back as it was.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so 'import'.length would theoretically better here, but yeah, feel free to add it in a new PR or we could use non-squash-merge specially for this PR and use two commits 🤔
For readability and understandment, I would much prefer to use 'import'.length here

Copy link
Member Author

@patak-dev patak-dev May 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the context is enough here, if we add something in another PR maybe it could be a hint in the comment above like 'import'.length === 6. I think this kind of patterns is not uncommon in the Vite code base though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I don't think it's a magic number, as it's quite clear to see it's the length of import and giving the previous comment saying "rewrite the import". 'import'.length would do but it's runtime, which to me, is not worth to bring the extra memory and CPU cost for the tiny DX that is not commonly touched anyway. I think it's ok to leave it as-is

Shinigami92
Shinigami92 previously approved these changes May 15, 2021
docs/config/index.md Outdated Show resolved Hide resolved
antfu
antfu previously approved these changes May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants