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: preserve order of link tags on HMR #982

Merged
merged 6 commits into from
Nov 29, 2022

Conversation

guansss
Copy link
Contributor

@guansss guansss commented Oct 21, 2022

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Resolves 959
Resolves #955

Problem:

Say there are two CSS chunks in sequence: A and B, the generated link tags appear like:

A.css
B.css

When A is modified, the HMR handler will append its new link tag to document.head, resulting in a different order:

B.css
A.css'

Now the order is incorrect - A's styles will override B's.

Solution:

Insert new link tag right after the old one, so that the result will be correct:

A.css'
B.css

I had no clue on how to add a test for this, 🤔 so I just updated the manual test, hope that's valid.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: guansss / name: Guan (b9cf364)

@guansss
Copy link
Contributor Author

guansss commented Nov 8, 2022

Hi @alexander-akait , could you please have a look at this?

@alexander-akait
Copy link
Member

@guansss Yeah, in my todo (just a lot of issues), will look tomorrow

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good

@alexander-akait
Copy link
Member

Can you update tests? Need to copy some files from test/fixtures/js to own tests

@guansss
Copy link
Contributor Author

guansss commented Nov 27, 2022

OK, updated.

@alexander-akait
Copy link
Member

Thank you

@guansss
Copy link
Contributor Author

guansss commented Nov 28, 2022

Hmm, there was an extra blank line generated while running tests on my machine, not sure why.

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Base: 90.37% // Head: 90.37% // No change to project coverage 👍

Coverage data is based on head (b8e28dc) compared to base (2633446).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #982   +/-   ##
=======================================
  Coverage   90.37%   90.37%           
=======================================
  Files           5        5           
  Lines         831      831           
  Branches      222      222           
=======================================
  Hits          751      751           
  Misses         70       70           
  Partials       10       10           
Impacted Files Coverage Δ
src/index.js 96.30% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alexander-akait
Copy link
Member

I have tested your solution with #959 and looks like it doesn't work, because order after HTML extraction is different

@guansss
Copy link
Contributor Author

guansss commented Nov 29, 2022

Oops, the issue I expected to solve is #955, I linked a wrong one, sorry for wasting your time to test on it :(

@guansss
Copy link
Contributor Author

guansss commented Nov 29, 2022

Updated files for old API test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Styles are out of order when HMR and async code splitting enabled
2 participants