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(plugin-legacy): add option to output only legacy builds #10139

Merged
merged 8 commits into from Jul 4, 2023

Conversation

lsdsjy
Copy link
Contributor

@lsdsjy lsdsjy commented Sep 16, 2022

Description

Fixes #9050

Additional context

This PR is a continuation of #9458 and adopts @sapphi-red 's suggestion. By controlling rollupOptions.output, the plugin skips modern bundle generating at all, which

  1. is faster;
  2. skips the unnecessary CSS generation. There's no need to generate separate .css files and <link> tags because legacy chunks will inline CSS text.

Besides, I have changed the option's name to renderModernChunks, which is easier to understand considering the existing renderLegacyChunks option.


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.

@lsdsjy lsdsjy changed the title Legacy no module feat(plugin-legacy): add option to output only legacy builds Sep 16, 2022
@sapphi-red sapphi-red added enhancement New feature or request plugin: legacy labels Sep 18, 2022
@Tal500
Copy link
Contributor

Tal500 commented Sep 24, 2022

2. skips the unnecessary CSS generation. There's no need to generate separate `.css` files and `<link>` tags because [legacy chunks will inline CSS text](https://github.com/vitejs/vite/blob/8f315a20a7c3a33c72e7993860b6b69f5d6f0e05/packages/vite/src/node/plugins/css.ts#L570-L583).

Please notice my PR #9920 that basically omits entirely CSS inlining for legacy, and instead (pre)load the same CSS chunks both for modern and legacy.

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Nov 21, 2022
@patak-dev
Copy link
Member

@lsdsjy sorry for the delay to review this PR. I've added it to our team board to be discussed in a future team meeting. Would you comment on your use case for this feature? Are you targeting a specific browser inside a private setup that can't be upgraded I imagine? If this is a public app, most users will benefit from our current default scheme.

To give visibility, there is a forked plugin in case this is needed before: npmjs.com/package/vite-plugin-legacy-no-module

@lsdsjy
Copy link
Contributor Author

lsdsjy commented Nov 21, 2022

Are you targeting a specific browser inside a private setup that can't be upgraded I imagine?

Yes. It's hard to explain though. It's important for us that the zipped archive of all dist files is small while still compatible with legacy devices, so we can't afford to have an extra set of dist files for modern devices.

To give visibility, there is a forked plugin in case this is needed before: npmjs.com/package/vite-plugin-legacy-no-module

I see. This PR had changes with vite core to enforce CSS inlining, so it was not feasible to only use a plugin. But thanks to #10496, a forked plugin can be a temporary workaround now. And I've reverted a99eade in my PR accordingly.

@Tal500
Copy link
Contributor

Tal500 commented Nov 21, 2022

Are you targeting a specific browser inside a private setup that can't be upgraded I imagine?

Yes. It's hard to explain though. It's important for us that the zipped archive of all dist files is small while still compatible with legacy devices, so we can't afford to have an extra set of dist files for modern devices.

To give visibility, there is a forked plugin in case this is needed before: npmjs.com/package/vite-plugin-legacy-no-module

I see. This PR had changes with vite core to enforce CSS inlining, so it was not feasible to only use a plugin. But thanks to #10496, a forked plugin can be a temporary workaround now. And I've reverted a99eade in my PR accordingly.

As a workaround, I can tell you that you can just delete all the output chunk files that doesn't ends up with -legacy.js.
Additionally, if you're prerendering the pages(or willing to introduce a middleware), you can remove the <script type="module" parts from HTML, and also remove the nomodule attribute from the `<script> tags, the same as done for Vite legacy automated tests:

// special test only hook
// for tests, remove `<script type="module">` tags and remove `nomodule`
// attrs so that we run the legacy bundle instead.
__test__() {
const indexPath = path.resolve(__dirname, './dist/index.html')
let index = fs.readFileSync(indexPath, 'utf-8')
index = index
.replace(/<script type="module".*?<\/script>/g, '')
.replace(/<script nomodule/g, '<script')
fs.writeFileSync(indexPath, index)
}

@patak-dev
Copy link
Member

Yes, @Tal500 has a point too. I don't know if we can justify the option just to speed up a bit plugin legacy here if there is a good way to remove the files after you build. We'll be discussing it in a future team meeting to see what is the best. The linked issues have several folks doing +1 to it, so maybe an option is indeed justified.

@lsdsjy
Copy link
Contributor Author

lsdsjy commented Nov 21, 2022

As a workaround, I can tell you that you can just delete all the output chunk files that doesn't ends up with -legacy.js. Additionally, if you're prerendering the pages(or willing to introduce a middleware), you can remove the <script type="module" parts from HTML, and also remove the nomodule attribute from the `<script> tags, the same as done for Vite legacy automated tests:

// special test only hook
// for tests, remove `<script type="module">` tags and remove `nomodule`
// attrs so that we run the legacy bundle instead.
__test__() {
const indexPath = path.resolve(__dirname, './dist/index.html')
let index = fs.readFileSync(indexPath, 'utf-8')
index = index
.replace(/<script type="module".*?<\/script>/g, '')
.replace(/<script nomodule/g, '<script')
fs.writeFileSync(indexPath, index)
}

Yes, it's basically what the prior work in #9458 does, while there's a little waste of computation.
So yes we have several workarounds, but it's better if we can integrate this into vite.

@aleen42
Copy link
Contributor

aleen42 commented Jan 5, 2023

any progress about this feature?

@patak-dev
Copy link
Member

Hey @aleen42, when there is progress in a PR or issue, we always comment on them so there is no need to ask that question (here or on other open-source projects)

In particular, this issue is in the backlog on our Team board, but we haven't had time to check it out yet.

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

LGTM

@zArubaru
Copy link

@sapphi-red Once check is dead!

@patak-dev patak-dev merged commit 931b24f into vitejs:main Jul 4, 2023
12 of 13 checks passed
@Kingwl
Copy link
Contributor

Kingwl commented Jul 4, 2023

💥💥💥

@sangquangnguyen
Copy link
Contributor

Hi @lsdsjy @Kingwl , could you try this feature yet? I used
legacy({ renderModernChunks: false }) then I will have the entry point index-legacy-123456.js in manifest.json, but when I used it, I will encounter the issue System is not defined, do you know this issue?
<script type="module" src="base_url/index-legacy-123456.js" />

@dante01yoon
Copy link

System is not defined

Same here, how can fix System is not defined

@sangquangnguyen
Copy link
Contributor

sangquangnguyen commented Aug 31, 2023

System is not defined

Same here, how can fix System is not defined

I found out the reason, I saw in manifest.json we have vite/legacy-polyfills-legacy file, we have to include this polyfill-legacy file into the script before the entry point to load all necessary polyfills. Hope that helps you @dante01yoon 👍

@dante01yoon
Copy link

System is not defined

Same here, how can fix System is not defined

I found out the reason, I saw in manifest.json we have vite/legacy-polyfills-legacy file, we have to include this polyfill-legacy file into the script before the entry point to load all necessary polyfills. Hope that helps you @dante01yoon 👍

thank you for comment, where manifest.json located?

@sangquangnguyen
Copy link
Contributor

System is not defined

Same here, how can fix System is not defined

I found out the reason, I saw in manifest.json we have vite/legacy-polyfills-legacy file, we have to include this polyfill-legacy file into the script before the entry point to load all necessary polyfills. Hope that helps you @dante01yoon 👍

thank you for comment, where manifest.json located?

In vite.config.js, you will have build props, then you need to define manifest: true, then run vite build. When Vite builds application successfully, you will see manifest.json inside build folder 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority) plugin: legacy
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

@vitejs/plugin-legacy support only output legacy bundle