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

optimizeForSpeed mode sometimes attempts to insert multiple rules as one #602

Open
jaydenseric opened this issue Nov 28, 2019 · 14 comments
Open

Comments

@jaydenseric
Copy link

@jaydenseric jaydenseric commented Nov 28, 2019

Do you want to request a feature or report a bug?

Bug.

What is the current behavior?

In optimizeForSpeed mode, sometimes multiple rules are being incorrectly stuffed into .insertRule() at once, here:

try {
sheet.insertRule(rule, index)
} catch (error) {
if (!isProd) {
console.warn(
`StyleSheet: illegal rule: \n\n${rule}\n\nSee https://stackoverflow.com/q/20007992 for more info`
)
}
return -1
}

In production the error is swallowed. This made it exceedingly hard for me to figure out why two seperate Next.js projects were having horrific style issues in production; the SSR page would be styled fine, but on client side route transitions, the styled-jsx styles for many of the components would be simply missing.

Here is a spectrum post from when I first encountered this bug a few weeks ago:

https://spectrum.chat/styled-jsx/general/absolutely-stumped-why-next-js-page-styles-broke~3ad888fa-23fc-4e6c-9948-fffa81a4e956

I found the "fix" at the time was to remove styling from a single component:

jaydenseric/device-agnostic-ui-website@a44fca1

Fast forward to now, and I have another project that suddenly started bombing out, here is a Zeit Now deployment:

whimsy-app-hhuy9ally.now.sh

If I put a console.error(error) in here, this is an example error:

Screen Shot 2019-11-28 at 8 15 19 pm

Here is a problematic "rule" that was being inserted:

.jsx-3944333314,.jsx-3103593668 ul,.jsx-3103593668 ol{margin-top:1em;margin-bottom:1.5em;padding-left:2em;}.jsx-3944333314:first-child,.jsx-3103593668 ul:first-child,.jsx-3103593668 ol:first-child{margin-top:0;}.jsx-3944333314:last-child,.jsx-3103593668 ul:last-child,.jsx-3103593668 ol:last-child{margin-bottom:0;}.jsx-3944333314>li,.jsx-3103593668 ul > li,.jsx-3103593668 ol > li{margin:0.5em 0;max-width:30em;line-height:var(--daui-line-height);}.jsx-3944333314>li:first-child,.jsx-3103593668 ul > li:first-child,.jsx-3103593668 ul > li:first-child{margin-top:0;}.jsx-3944333314>li:last-child,.jsx-3103593668 ul > li:last-child,.jsx-3103593668 ol > li:last-child{margin-bottom:0;}

It was the result of a few weeks of work without a production deployment, so unfortunately I can't say exactly what change pushed the bug to occur. I haven't lucked out on a miraculous "fix" this time though, so after MANY hours of debugging I'm getting pretty desperate!

If the current behavior is a bug, please provide the steps to reproduce and possibly a minimal demo or testcase in the form of a Next.js app, CodeSandbox URL or similar

I'm sorry I only have complicated, real world examples linked above and I haven't been able to isolate a minimal reproduction.

I have discovered that debugging is a lot easier when running optimizeForSpeed in dev, so traces aren't minified:

{
  "presets": [["next/babel", { "styled-jsx": { "optimizeForSpeed": true } }]]
}

What is the expected behavior?

Styles managed via styled-jsx in a Next.js app render correctly on the client after route transitions.

Environment (include versions)

  • OS: macOS
  • Browser: Chrome v78
  • styled-jsx (version): v3.2.4

Did this work in previous versions?

Not sure.

@giuseppeg

This comment has been minimized.

Copy link
Collaborator

@giuseppeg giuseppeg commented Nov 28, 2019

@jaydenseric hey mate I am sorry that you are experiencing this kind of issue! Sounds super frustrating. Before we even try to debug, can you make sure that both the library and the consumer app are transpiled in the same "mode" (either NODE_ENV production or development).

The issue could be that your components library is compiled for development mode and when the Next.js app is deployed to prod it instead uses optimizeForSpeed which cannot insert styles compiled for dev mode.

@jaydenseric

This comment has been minimized.

Copy link
Author

@jaydenseric jaydenseric commented Nov 28, 2019

I'll check, but if that is the case, how come none of the styles are broken right now at https://deviceagnosticui.com, or https://jaydenseric.com?

@giuseppeg

This comment has been minimized.

Copy link
Collaborator

@giuseppeg giuseppeg commented Nov 28, 2019

it seems that you are not using optimizeForSpeed there

Screen Shot 2019-11-28 at 11 16 27 AM

@giuseppeg

This comment has been minimized.

Copy link
Collaborator

@giuseppeg giuseppeg commented Nov 28, 2019

this article explains how to bundle a library that uses styled-jsx to avoid this issue https://medium.com/@tomaszmularczyk89/guide-to-building-a-react-components-library-with-rollup-and-styled-jsx-694ec66bd2

@jaydenseric

This comment has been minimized.

Copy link
Author

@jaydenseric jaydenseric commented Nov 28, 2019

Thanks for the revelation, although this is a real headache to solve elegantly 😓

Bundling published packages is an antipattern, and you can't have require() inside ESM for a conditional export. ESM for tree-shaking is an important feature for a published a component library, so I don't want to drop it. Publishing invalid ESM is undesirable because eventually, maybe even in January when conditional package exports are shipped by Node.js, I'll be publishing ESM intended to be run natively.

I think I might publish two builds at device-agnostic-ui for development and device-agnostic-ui/production for production, then have Babel/Webpack config to setup in Next.js to rewrite the imports accordingly.

@jaydenseric

This comment has been minimized.

Copy link
Author

@jaydenseric jaydenseric commented Nov 28, 2019

Is optimizeForSpeed even worth it? What exactly are the benefits?

I might just continue to publish device-agnostic-ui components and styles transpilled with it false, and in the Next.js setup instructions explain that the Babel config needs to be configured like this:

{
  "presets": [["next/babel", { "styled-jsx": { "optimizeForSpeed": false } }]]
}
@giuseppeg

This comment has been minimized.

Copy link
Collaborator

@giuseppeg giuseppeg commented Nov 28, 2019

optimizeForSpeed uses the CSSOM apis which are way faster than creating script tags.

As for bundling you have a few options:

  1. Ship the source code and let the host app build it
  2. You can use conditionals, bundlers will treeshake this https://github.com/giuseppeg/style-sheet/blob/aa125093adcef82ca3bd86b58f5f203ef232a4fa/lib/esm/createElement.js (works only if the bundler replaces process.env.NODE_ENV === 'production' with a constant, eg. webpack does that)
@giuseppeg

This comment has been minimized.

Copy link
Collaborator

@giuseppeg giuseppeg commented Nov 28, 2019

  1. won't work in UMD or browser of course
@jaydenseric

This comment has been minimized.

Copy link
Author

@jaydenseric jaydenseric commented Nov 29, 2019

Here is an example of the difference between transpiling published components without and with NODE_ENV=production

For this component:

https://deviceagnosticui.com/components/Blockquote

Here is without:

export var stylesBlockquote = {
  styles: React.createElement(
    _JSXStyle,
    {
      id: '3946479178'
    },
    '.jsx-3946479178,.' +
      stylesHtml.className +
      ' blockquote{margin-top:1.5em;margin-bottom:1.5em;margin-left:1.5em;max-width:22em;line-height:var(--daui-line-height);font-style:italic;font-weight:300;text-rendering:optimizeLegibility;}.jsx-3946479178:first-child,.' +
      stylesHtml.className +
      ':first-child{margin-top:0;}.jsx-3946479178:last-child,.' +
      stylesHtml.className +
      ':last-child{margin-bottom:0;}'
  ),
  className: 'jsx-3946479178'
}

Here is with:

export var stylesBlockquote = {
  styles: React.createElement(
    _JSXStyle,
    {
      id: '3946479178'
    },
    [
      '.jsx-3946479178,.' +
        stylesHtml.className +
        ' blockquote{margin-top:1.5em;margin-bottom:1.5em;margin-left:1.5em;max-width:22em;line-height:var(--daui-line-height);font-style:italic;font-weight:300;text-rendering:optimizeLegibility;}',
      '.jsx-3946479178:first-child,.' +
        stylesHtml.className +
        ':first-child{margin-top:0;}',
      '.jsx-3946479178:last-child,.' +
        stylesHtml.className +
        ':last-child{margin-bottom:0;}'
    ]
  ),
  className: 'jsx-3946479178'
}

The only difference appears to be the rules are one big string in dev, and an array of a string for each rule for prod.

Could styled-jsx be enhanced to automatically .join() an array of rules encountered in development? That way component libraries could be published only transpilled for production.

jaydenseric added a commit to jaydenseric/device-agnostic-ui-website that referenced this issue Nov 29, 2019
@giuseppeg

This comment has been minimized.

Copy link
Collaborator

@giuseppeg giuseppeg commented Nov 29, 2019

Could styled-jsx be enhanced to automatically .join() an array of rules encountered in development?

@jaydenseric it is not possible to mix up optimizedForSpeed styles with non optimizedForSpeed. With optimizedForSpeed we insert to a single style tag using the CSSOM API, when it is disabled we create multiple style tags.

We can add a warning here though

add(props) {
if (undefined === this._optimizeForSpeed) {
this._optimizeForSpeed = Array.isArray(props.children)
this._sheet.setOptimizeForSpeed(this._optimizeForSpeed)
this._optimizeForSpeed = this._sheet.isOptimizeForSpeed()
}

like if this._optimizeForSpeed is true but props.children (the styles) are not an array print a warning

@jaydenseric

This comment has been minimized.

Copy link
Author

@jaydenseric jaydenseric commented Nov 29, 2019

Perhaps you misunderstood the suggestion...

It would be impossible to support importing non optimizedForSpeed styles for use in a production build, because it is hard to cut up a style string into an array of rules at runtime - that should always happen build time.

But, I can't see why the other way around can't be supported. You should be able to import optimizedForSpeed styles for use in a development build, because it would be trivial at runtime to detect the styles are an array of rules, and simply array .join() them together into a single string, just like how non optimizedForSpeed styles are normally.

With this behavior, to write a component/styles library all you need to do is publish a single optimizedForSpeed build. This would make it much easier to publish styled-jsx styles and a native ESM build would be possible.

jaydenseric added a commit to jaydenseric/graphql-react-examples that referenced this issue Dec 2, 2019
jaydenseric added a commit to jaydenseric/apollo-upload-examples that referenced this issue Dec 2, 2019
@giuseppeg

This comment has been minimized.

Copy link
Collaborator

@giuseppeg giuseppeg commented Dec 17, 2019

@jaydenseric that is a bit "magic" but it might work, want to put together a PR?

@jaydenseric

This comment has been minimized.

Copy link
Author

@jaydenseric jaydenseric commented Dec 17, 2019

@giuseppeg I got started on a PR a few weeks ago but had to move onto other stuff. Sorry, but I'm now out of time and money to work on OSS, see https://twitter.com/jaydenseric/status/1206800095280128006. I've been working on OSS pro bono full time since August; time to get back into paid work.

@giuseppeg

This comment has been minimized.

Copy link
Collaborator

@giuseppeg giuseppeg commented Dec 17, 2019

@jaydenseric no worries! I get that as I am not paid to maintain any of these OSS projects as well :)

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.