Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(html): move importmap before module scripts #9392

Merged
merged 14 commits into from
Aug 21, 2022
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jul 27, 2022

Description

fix #9334

Arrange script type="importmap" as first in transformIndexHtml.

Additional context

Needed this for dev external tests 🙂


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.

@bluwy bluwy added feat: html p3-minor-bug An edge case that only affects very specific usage (priority) labels Jul 27, 2022
@patak-dev
Copy link
Member

Nice fix. I'm leaning towards the alternative proposal though. IIUC this PR could end up reordering styles and scrpts, no?

@bluwy
Copy link
Member Author

bluwy commented Jul 27, 2022

This PR currently reorders script type-"module" only if an importmap exist, so other cases it should work like before. Also happy to try out the alternative though.

@sapphi-red
Copy link
Member

I'm leaning towards the alternative one, too. I think it is more easy to understand the behavior, especially when an importmap is injected by transformIndexHtml.

Not sure if this would be surprising for users though.

I think a warning will make it less surprising.
For example, when the input index.html has <script type="module"> before importmap.

@bluwy
Copy link
Member Author

bluwy commented Jul 27, 2022

Updated to use a Vite plugin with transformIndexHtml to move the importmap instead. I didn't add a warning though as I think there are cases where someone has no control over where the importmap is injected in a Vite plugin (?).

Also avoiding traverseHtml to save a bit of perf.

@sapphi-red
Copy link
Member

I didn't add a warning though as I think there are cases where someone has no control over where the importmap is injected in a Vite plugin (?).

I was thinking of when the input of transformIndexHtml is invalid, not during the transformIndexHtml.
Example: https://stackblitz.com/edit/vitejs-vite-e3cqtr?file=index.html&terminal=dev
This one is invalid and won't work before this PR but IIUC this works after this PR.

@bluwy
Copy link
Member Author

bluwy commented Jul 27, 2022

Yeah I think that could be something that we can warn by default. Would have to warn before any transfromIndexHtml happens 🤔 I'll see if I can add that tomorrow.

@bluwy bluwy changed the title fix: prepend module scripts after importmap fix: move importmap before module scripts Jul 28, 2022
@bluwy bluwy changed the title fix: move importmap before module scripts fix(html): move importmap before module scripts Jul 28, 2022
bluwy and others added 2 commits July 29, 2022 20:28
Co-authored-by: 翠 / green <green@sapphi.red>
@patak-dev patak-dev added this to the 3.1 milestone Jul 29, 2022
Co-authored-by: 翠 / green <green@sapphi.red>
sapphi-red
sapphi-red previously approved these changes Jul 30, 2022
@patak-dev patak-dev merged commit b386fba into main Aug 21, 2022
@patak-dev patak-dev deleted the fix-importmap-prepend branch August 21, 2022 17:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
feat: html p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Vite inject "/@vite/client" script element above importmap element
3 participants