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

extract render-html-plugin #855

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cortopy
Copy link

@cortopy cortopy commented Jul 29, 2019

What kind of change does this PR introduce?

It extracts render-html-plugin into a separate package.

Did you add tests for your changes?

These changes do not include any change in the logic of the application. This PR limits itself to move existing files into separate packages. I have tested the changes by creating and building an app using this version.

The only change in codebase is the currying of the plugin with a config object and the introduction of a find logic for the template file.

If this PR looks ok to reviewers, I'm happy to add more tests.

Summary

The main goal for doing this is to allow more customisation of the prerender process as per #844

For final users, this PR does not change much as it doesn't expose an easy way to configure the plugin. I think this would be better addressed in a future PR, since this one already introduces some changes that I would like to have reviewed first:

  • Move the files related to the prerender process (in util/webpack) into a separate package. This would require publishing this package.
  • These files depend on an util file which now becomes a shared dependency between the main cli package and @preact/render-html-plugin. For this reason, I have also created a @preact/cli-util package so that both can share the code. This would also require publishing the package

Does this PR introduce a breaking change?
No

Other information

Please paste the results of preact info here.

@ForsakenHarmony
Copy link
Member

I'm still not sure what exactly you'd like to be able to customise

@cortopy
Copy link
Author

cortopy commented Aug 7, 2019

I'm still not sure what exactly you'd like to be able to customise
@ForsakenHarmony

Instead of css modules, I use a CSS-in-JS library. As a result, I need the prerendering to output two strings, one for the HTML body and another for the styles.

I've started using my forked solution. But anybody that uses js libraries for styles, regardless of which one, would have to do something similar. Otherwise the prerendered pages would have no styles at all.

This is just one example of why prerendering can't fit into one-size-for-all solution. I wouldn't expect preact-cli to cover for all possible scenarios. My case is about styles, but I'm pretty sure others will have other needs.

Extracting the process as a plugin is, in my opinion, the first step towards achieving customisation using the webpack preact.config.js file

@ForsakenHarmony
Copy link
Member

While I do think we should support your use case I think we might want to do so at a different point, maybe make the ssr function itself customisable

@cortopy
Copy link
Author

cortopy commented Aug 11, 2019

@ForsakenHarmony that's great to hear. The main point of this PR and the steps I tried to plan in #844 are already aimed in that direction.
For what you're saying I understand that you would rather have a different approach to this PR? If so, could you please explain so that I can modify accordingly?

If this is something that the core team would prefer doing themselves, please also let me know so that I close this PR and carry on using my fork.

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.

2 participants