frandiox / vite-ssr

Use Vite for server side rendering in Node
MIT License
824 stars 91 forks source link

fix: Split utils/state.ts into utils/serialize-state.ts & utils/deserialize-state.ts. Closes #118 #119

Closed salqueng closed 2 years ago

salqueng commented 2 years ago

~~This PR replaces regexes to use lookahead expressions instead of lookbehind expressions. ~~

Safari considers lookbehind regexes are invalid, so current serializeState method raises SyntaxError. So, to fix the problem, regexes without lookbehind expressions are required.

Safari considers lookbehind regexes are invalid, so current serializeState method raises SyntaxError. Split state.ts into two files: serialize-state.ts and deserialize-state.ts, and use serialize-state.ts in entry-server.ts, and use deserialize-state.ts in entry-client.ts

frandiox commented 2 years ago

@salqueng Thanks a lot for taking the time! I didn't know this was possible with a lookahead expression 😮

I was reviewing this when I remembered something: serializeState never runs in the browser, it only runs in the server. The problem we have is that Vite downloads the whole state.ts file in development and Safari evaluates the regex syntax. It works in prod because serializedState is not bundled in the client build.

Apart from that, I've checked the performance between lookahead and lookbehind, and looks like lookbehind is 30% faster -- and more readable/ easier to maintain.

image

Therefore, I'd suggest keeping the lookbehind approach but split this file in two: serialize-state.ts and deserialize-state.ts. This way, Vite won't push the regexps to the browser in development.

Would you like to update this PR with those changes? Thanks!

salqueng commented 2 years ago

Thank you for the response! You're absolutely right - serializeState is not for clients, so splitting state.ts is the best option. I'll update it after this afternoon. :D