Navigation Menu

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

Jump to bottom

fix: sequential injection of tags in transformIndexHtml (#5851) #6901

Merged
merged 3 commits into from Jun 14, 2022
Merged

fix: sequential injection of tags in transformIndexHtml (#5851) #6901

merged 3 commits into from Jun 14, 2022

Conversation

Menci
Copy link
Contributor

@Menci Menci commented Feb 13, 2022

Description

The built-in html plugin provides a transformIndexHtml hook for all plugins, which inputs contents of index.html and outputs modified contents and some tags to inject. But all tags to inject are injected after all plugins' transformIndexHtml hook, so tags injected by previous plugins are not visible to next ones.

It closes #5851.

Additional context

I changed the code to handle tags injection after each plugin's hook, so the performance inevitably degrades. Is it acceptable? If not, should we use other ways such as adding another parameter to transformIndexHtml to receive tags that previous plugins want to inject?

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.

@Menci Menci mentioned this pull request Feb 14, 2022
Closed
@tyouzu1
Copy link
Contributor

tyouzu1 commented Apr 15, 2022

same question, can't wait.

@Menci
Copy link
Contributor Author

Menci commented Apr 15, 2022

@tyouzu1 As a workaround, you can currently use the generateBundle hook. In this hook you can get the "final" HTML and rewrite it.

e.g. https://github.com/Menci/vite-plugin-public-path/blob/f518c133011a4105d83681ea351d3f2c4a718b65/src/index.ts#L103

@tyouzu1
Copy link
Contributor

tyouzu1 commented Apr 15, 2022

@tyouzu1 As a workaround, you can currently use the generateBundle hook. In this hook you can get the "final" HTML and rewrite it.

e.g. https://github.com/Menci/vite-plugin-public-path/blob/f518c133011a4105d83681ea351d3f2c4a718b65/src/index.ts#L103

Already seen. No test yet. Thanks anyway
But here,I saw #5851 (comment) say “it could be a breaking change”. Maybe should find other ways

@bluwy bluwy added feat: html p2-to-be-discussed Enhancement under consideration (priority) labels Apr 19, 2022
@patak-dev
Copy link
Member

I think this makes sense, it is already added for discussion in a team meeting. We'll try to get to it before the v3 release.

@patak-dev patak-dev added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) p3-significant High priority enhancement (priority) and removed p2-to-be-discussed Enhancement under consideration (priority) p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels May 18, 2022
@sapphi-red sapphi-red added this to the 3.0 milestone Jun 14, 2022
@sapphi-red
Copy link
Member

sapphi-red commented Jun 14, 2022 •
edited

Just to clarify. This PR changes the order slightly.

For example, if there are two plugins like this:

{
  plugins: [
    {
      name: 'hook1',
      transformIndexHtml(html) {
        return [
          { tag: 'meta', children: 'hook1-pre', injectTo: 'head-prepend' },
          { tag: 'meta', children: 'hook1', injectTo: 'head' }
        ]
      }
    },
    {
      name: 'hook2',
      transformIndexHtml(html) {
        return [
          { tag: 'meta', children: 'hook2-pre', injectTo: 'head-prepend' },
          { tag: 'meta', children: 'hook2', injectTo: 'head' }
        ]
      }
    }
  ]
}
<!-- output before this PR -->
<head>
<meta>hook1-pre</meta>
<meta>hook2-pre</meta>
<meta>hook1</meta>
<meta>hook2</meta>
</head>

<!-- output after this PR -->
<head>
<meta>hook2-pre</meta>
<meta>hook1-pre</meta>
<meta>hook1</meta>
<meta>hook2</meta>
</head>

patak-dev
patak-dev approved these changes Jun 14, 2022
View reviewed changes
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

The new order is what I would expect 👍🏼
I would consider the order change a bug fix.

@patak-dev patak-dev changed the title fix(plugins/html): fix tags injected by previous plugins not visible to next (#5851) fix: sequential injection of tags in transformIndexHtml (#5851) Jun 14, 2022
@patak-dev patak-dev merged commit 649c7f6 into vitejs:main Jun 14, 2022
sapphi-red added a commit to sapphi-red/vite that referenced this pull request Jun 14, 2022
@sapphi-red
docs: mention breaking change from vitejs#6901
be126f8
sapphi-red added a commit to sapphi-red/vite that referenced this pull request Jun 14, 2022
@sapphi-red
docs: mention breaking change from vitejs#6901
e7c33ee
@sodatea sodatea mentioned this pull request Jul 25, 2022
Closed
7 tasks
@lmiller1990 lmiller1990 mentioned this pull request Jul 25, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patak-dev patak-dev patak-dev approved these changes

@sapphi-red sapphi-red sapphi-red approved these changes

Assignees
No one assigned
Labels
breaking change feat: html p3-significant High priority enhancement (priority)
Projects
Team Board
Archived in project
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

The transformIndexHtml hook can't get the real html in the context.
5 participants
@Menci @tyouzu1 @patak-dev @sapphi-red @bluwy