-
Notifications
You must be signed in to change notification settings - Fork 616
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: hydrate in twind-v1-plugin #1050
fix: hydrate in twind-v1-plugin #1050
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Is it possible to write a test for this fix?
@ry Yes, I will do it soon!
|
Thank you!
just
Yes, it would be helpful! |
@hashrock Thanks! |
I added test!
Please let me know if I am missing or wrong! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test. I think this looks good but it could use some clean up still.
@ry Thanks for the review! But there is one thing I couldn't solve. Lines 23 to 25 in c4cfb20
I have tried calling cancel() of lines and externally TextDecoderStream (ex. const decoder = TextDecoderStream(); ), but could't solve it.
For now, I got the textDecoder's rid from the resourceMap and manually close the resource. Lines 47 to 63 in c4cfb20
Is there a correct cancel() call? |
Without |
@hashrock Thanks for merging! |
This PR is a slightly modified version of hydrate from twind-v1-plugin (#946).
In this plugin, Cssrules of SSR are resumed by cssom() of twind.
There are demos for each of #946 and this PR. (I added a lot of classes to compare the performance.)
feat: add twind v1 plugin #946 plugin's Demo (source)
This PR plugin's Demo (source)
What is the issue with #946 plugin?
Twind's getSheet returns the sheet created from cssom to which resume is assigned, but since SSR's HtmlStyleSheet cannot be targeted to this sheet, resume at setup doesn't make sense.
![pr946-demo-styles](https://user-images.githubusercontent.com/12783604/221395087-4f28afaf-c166-4c17-a991-73c4c863f437.png)
As a result, CSSRules of ClientSide that partially overlaps with ServerSide is inserted.
Demo
I think this should be improved.
What was changed?
I made a sheet created by cssom with SSR's HtmlStyleSheet as the target, and bind the resume of the sheet created by getSheet.
Demo
![thisPR-demo-styles](https://user-images.githubusercontent.com/12783604/221395750-9d0bc6c2-9226-46fe-aa46-c19aa533435e.png)
The resume of the sheet is working, so there is no duplication.
What about performance?
I compared the performance of each plugin on chrome.
The Scripting time for each is about the same.
Thank you very much for reading through to the end.
Please let me know if my understanding is wrong.🙏