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

WIP: Static rendering #330

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

WIP: Static rendering #330

wants to merge 22 commits into from

Conversation

okonet
Copy link
Member

@okonet okonet commented Feb 18, 2017

This PR implements static rendering of the styleguide when building for production.

🚧🚧🚧
Please don't merge yet. Still WIP!!!
🚧🚧🚧

@okonet okonet self-assigned this Feb 18, 2017
@@ -0,0 +1 @@
**/styleguide/*.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have rules in global gitignore:

/examples/**/styleguide/build
/examples/**/styleguide/index.html

Scripts should be in a separate folder anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Is there a reason for them to be in a separate dir BTW?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better UI: not to scare users with extra files and make index.html easier to find ;-)

@okonet
Copy link
Member Author

okonet commented Feb 21, 2017

It looks like I was able to render everything besides the actual components (which is by design) statically using latest commit.

This includes:

  • TOC (Table of Contents) with proper slugs (this took some time)
  • Markdown documents
  • Code highlighted with highlight.js

We'll need to take some care of injecting CSS into head during the build time as well.

package.json Outdated
@@ -58,15 +57,18 @@
"loader-utils": "^0.2.16",
"lodash": "^4.17.4",
"minimist": "^1.2.0",
"node-noop": "^1.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lodash/noop should be enough ;-)

scripts/build.js Outdated
// require('fs').writeFileSync('stats.json', JSON.stringify(stats.toJson()));
callback(err, stats);
// Firstly, build static "server" bundle
return webpack(makeWebpackConfig(config, 'production', true), (err, stats) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pass an array of webpack config here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on passing an array here? What do you mean exactly? How would this change it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand you can call webpack like this: webpack([config1, config2]).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this isn't the same. This will produce 2 compiler's processes but we want to share one process.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you share it? I see two separate webpack calls with different configs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you might be right about it. I was a bit confused. Let me try it out.

import React, { PropTypes } from 'react';
import HTMLDocument from 'react-html-document';

function getScrtips(assets) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in HtmlDocument.js. The idea is to provide a simple way to redefine rendering of components, so data and markup should be separated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! Will refactor appropriately.

{ name: 'charset', content: 'utf-8' },
]}
>
{ children }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m wondering why ESLint don’t complain about this spaces… 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No rule defined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll check ;-)

@okonet
Copy link
Member Author

okonet commented Feb 23, 2017

@kof can you help me here with rendering styles statically? SG isn't using react-jss so no SheetsRegistry etc.

BTW @sapegin should we switch to react-jss? Can you elaborate in more details what's is missing from it to be able to use the "official" repository?

@codecov-io
Copy link

codecov-io commented Feb 23, 2017

Codecov Report

Merging #330 into master will decrease coverage by <.01%.
The diff coverage is 97.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
- Coverage   94.77%   94.77%   -0.01%     
==========================================
  Files          75       78       +3     
  Lines         995     1014      +19     
  Branches      202      211       +9     
==========================================
+ Hits          943      961      +18     
- Misses         52       53       +1
Impacted Files Coverage Δ
scripts/schemas/config.js 96.96% <ø> (ø) ⬆️
scripts/make-webpack-config.js 100% <100%> (ø) ⬆️
loaders/utils/getStyleguide.js 100% <100%> (ø)
scripts/create-server.js 90.9% <100%> (ø) ⬆️
...sg-components/HtmlDocument/HtmlDocumentRenderer.js 100% <100%> (ø)
loaders/styleguide-loader.js 100% <100%> (ø) ⬆️
src/rsg-components/HtmlDocument/HtmlDocument.js 100% <100%> (ø)
...rc/rsg-components/ReactComponent/ReactComponent.js 100% <100%> (ø) ⬆️
src/rsg-components/Examples/Examples.js 83.33% <100%> (ø) ⬆️
src/utils/utils.js 96.92% <85.71%> (-1.44%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f80d73...c82a157. Read the comment docs.

@kof
Copy link

kof commented Feb 23, 2017

@okonet I am not aware of decision details, it seems like react-jss would be a good fit here. In any case its easy. You can always create a SheetsRegistry and add the sheets to it and then get them all together as a CSS string.

- Use proper `charSet` key for the metatag as of venmo/react-html-document#9
- Moved metatags to renderer to allow easy customization without merging
@okonet
Copy link
Member Author

okonet commented Feb 23, 2017

@kof can you commit the solution you're suggesting?

@sapegin
Copy link
Member

sapegin commented Feb 23, 2017

@okonet @kof Basically these two lines. We need to merge styles with user’s styles and and also pass merged theme object. And strip Renderer for nicer class names.

I’m not sure it’s all possible to do with react-jss and what would be the best solution.

@kof
Copy link

kof commented Feb 23, 2017

Btw. WrappedComponent.name is not supported everywhere, haven't found caniuse data in this regard.

@kof
Copy link

kof commented Feb 23, 2017

react-jss uses options.meta property if you pass it, otherwise it falls back to a component name https://github.com/cssinjs/react-jss/blob/master/src/index.js#L36

@kof
Copy link

kof commented Feb 23, 2017

Why do you call them SomethingRenderer anyways and then replace the Renderer part in order to have nicer meta attributes on style tags? Why not to remove the Renderer thing from the name from the beginning? If it is not a container component, it is a renderer anyways, containers have no sheets.

@kof
Copy link

kof commented Feb 24, 2017

Ok, I have finished the SSR integration for JSS. I needed to do 2 suboptimal things, because of "react-html-document" component limitations.

  1. We need to get the styles used by the application before we render the document. That module assumes we don't. So I needed to render the app to string first and then pass it to HtmlDocument inside of a div with dangerouslySetInnerHTML.
  2. HtmlDocument abstraction doesn't give any option to define an attribute on server-side rendered style tag, in order to delete that tag once js kicks in I currently remove the first tag, but it will break if you add more style tags on the server and they will come first.

@okonet
Copy link
Member Author

okonet commented Feb 24, 2017

Hmm, that makes me think if we still should use HtmlDocument. Right now it looks as hack. Are there any better options to do that @kof?

@kof
Copy link

kof commented Feb 24, 2017

Yes, simple template string like here

@kof
Copy link

kof commented Feb 24, 2017

Or optionally own document renderer component without that dependency.

@okonet
Copy link
Member Author

okonet commented Feb 24, 2017

How own renderer would help us here?

@kof
Copy link

kof commented Feb 24, 2017

How own renderer would help us here?

  • It would specify an id for the ssr style tag.
  • It would accept content as an html string and incapsulate dangerouslySetInnerHTML

@kof
Copy link

kof commented Feb 24, 2017

Optionally this can be done by sending a pr to that original package and make this possible.

@okonet
Copy link
Member Author

okonet commented Feb 24, 2017

@kof do you mind updating the PR by removing the react-html-document and introducing the template string? I'm away for a few days so won't be able to work a lot. 🏂

@kof
Copy link

kof commented Feb 24, 2017

I think I can make it happen

@sapegin
Copy link
Member

sapegin commented Feb 24, 2017

@kof Awesome!

@okonet
Copy link
Member Author

okonet commented Mar 7, 2017

@kof any news on this one?

@kof
Copy link

kof commented Mar 7, 2017

@okonet not yet, could find time right now, because on the trip

@okonet
Copy link
Member Author

okonet commented Apr 12, 2017

@sapegin NOOOO! Why did you close it?

@sapegin
Copy link
Member

sapegin commented Apr 12, 2017

I’ve removed the next branch because it was merged, so it wasn’t me ;-| Is it possible to change the target branch without resending a PR?

@okonet
Copy link
Member Author

okonet commented Apr 12, 2017


It actually worked :) Still needs a rebase.

@okonet okonet changed the base branch from next to master April 12, 2017 08:05
@okonet okonet reopened this Apr 12, 2017
@okonet
Copy link
Member Author

okonet commented Aug 18, 2017

@kof do you think you could find some time to get this done any soon?

@sapegin
Copy link
Member

sapegin commented Aug 18, 2017

We probably need to create another branch first because this one is probably from 4.x and it would be a nightmare to merge master to it.

@kof
Copy link

kof commented Aug 18, 2017

Will be hard, I have a great deal of issues on jss project.

@FDiskas
Copy link

FDiskas commented May 25, 2023

Can be closed probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants