-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add parsing when using SSR. Fixes #6. #18
Conversation
Bump nodejs version for travis to current LTS.
I have no idea why one of the tests is failing in travis. Can't reproduce on my local machine. |
@developit Is there any chance you could have a look and tell me, what you think? I can make adjustments, but I really think, SSR should be fixed. |
This shouldn't be merged ... it will add unnecessary stuff for the client side if you use webpack. Better add to your SSR code: I often use this snipped..
|
I kind of have to agree, since in the meantime, I don't think dom-parser is actually a very good implementation. However, I don't really understand your workaround? Can you elaborate? Can we avoid having dom-parser in the bundle? I also don't really understand why the package increases your production bundle size so heavily when it just slightly increases the compressed size of this component alone? |
The global stuff is for my client bundle because i use web based stuff that doesn't exist in node like IndexedDB, fetch from my store, DOMParser for preact-markup. My setup looks like: (client)
polyfill bundle uses: (old clients)
In my SSR lambda i use Inside of
For generating the body you can just do `render(h(App, { url: e.path }))
will take the rest ... Maybe i could build a boilerplate for preact-cli next weekend's if there is a need for. |
Thanks for your response! So what I basically take from your explanation is, you're building different bundles for client and server and in the server bundle you make browser-related stuff available through globals you're defining. While this sounds great, I don't think it's a solution for everyone. I would urge @developit simply to decide if this is a feature the component should support and then it's probably possible to polyfill with a small footprint, or have a second export for the ssr-supported version, so that existing bundles stay small and others can make use of server-side-rendering. By not responding to this pull-request for such a long I time, I guess the author already made a decision to not support this. I've changed to a React module that works on both, client and server. |
Apologies for not responding to your PR @te-online! It wasn't intentional, I just get too many Github notifications so things often slip through unnoticed. I would really like to support this use-case. Anyone using preact-cli is likely to run into the issue you ran into, including once we migrate preactjs.com over to the CLI. One option I had thought of: what about exporting the server-side parser as a function, and requiring that during SSR it is passed into import Markup from 'preact-markup';
// etc
render() {
return (
// instead of bundling the parser, allow it to be passed in:
const parser = typeof document==='undefined' && require('preact-markup/server');
<Markup parser={parser} markup="html here" />
);
} Otherwise I actually think this PR is the right way to go: |
Thank you so much for your response! 😊 I like the idea of passing in the parser if necessary. 👍 Unfortunately, I will not be able to update the PR in the upcoming weeks. I could add it to my backlog, though, but would be more than |
@developit Your suggestion led me to follow a different approach, where we would leave the choice of parser to the developer using the component. So we can avoid the awkward parser decision on the module side and at the same time provide some flexibility. I don't know if this is a sensible solution 😄 See #22 |
FWIW, Preact WMR uses JSDOM, see: #6 (comment) |
Adds dom-parser (don't know if you're okay with this module?) to fix Server-Side-Rendering. I don't think we would want different builds for server and browser, so we always need to the module.
Couldn't add any tests for this, because Karma only works in the browser. Any ideas regarding testing?
Increases the gzipped size from 2.5 kB to 2.6 kB.