-
Notifications
You must be signed in to change notification settings - Fork 53
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: catastrophic backtracking and esbuild platform #238
Conversation
Codecov Report
@@ Coverage Diff @@
## master #238 +/- ##
=======================================
Coverage 82.14% 82.14%
=======================================
Files 46 46
Lines 1960 1960
Branches 465 440 -25
=======================================
Hits 1610 1610
Misses 348 348
Partials 2 2
Continue to review full report at Codecov.
|
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.
Thank you so much for the PR.
Could I ask a test to cover the regex change? It is clear that you've got a deep understanding of the issue and your regex is better than mine. 👍
Ready to go besides that.
Thanks for all of the support on the project.
@@ -155,7 +155,7 @@ const svelteHandler = async ({ elderConfig, svelteConfig, replacements, restartH | |||
}, | |||
format: 'esm', | |||
target: ['es2020'], | |||
platform: 'node', | |||
platform: 'browser', |
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.
So obvious. Thank you for the good catch here.
@@ -35,7 +35,7 @@ describe('#mountComponentsInHtml', () => { | |||
|
|||
mountComponentsInHtml.default({ | |||
page, | |||
html: `<div class="svelte-datepicker"><div class="ejs-component" data-ejs-component="Datepicker" data-ejs-props="{ "a": "b" }" data-ejs-options="{ "loading": "lazy" }"></div></div>`, | |||
html: `<div class="svelte-datepicker"><div class="ejs-component" data-ejs-component="Datepicker" data-ejs-props="{ "a": "b" }" data-ejs-options="{ "loading": "lazy" }"></div></div>`, |
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.
This is indeed correct as the props and options come out of JSON.stringify
and escapeHtml
in inlineSvelteComponent.ts
then are reversed during the mounting process. TY
A performance test is added. You can observe the performance issue with following commands: # run the performance test
npx jest src/partialHydration/__tests__/mountComponentsInHtml.spec.ts
# use old regex and run again. It will hang and eat up your CPU
git checkout master -- src/partialHydration/mountComponentsInHtml.ts
npx jest src/partialHydration/__tests__/mountComponentsInHtml.spec.ts |
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.
Looks great. Thank you.
Fixes #235
Fixes #219