feat: __webpack_nonce__ support in SSR + Browser contexts #1022

quantizor · 2017-07-22T16:56:06Z

Closes #887

quantizor · 2017-07-22T16:56:37Z

src/models/ServerStyleSheet.js

+    html += '>'
+
+    /* eslint-disable */
+    const css = Object.keys(this.components).reduce((styles, key) => {


was able to remove an array iteration here

quantizor · 2017-07-22T16:57:38Z

src/test/ssr.test.js

+    const css = format(sheet.getStyleTags())
+
+    expect(html).toEqual('<h1 class="sc-a b" data-reactroot="" data-reactid="1" data-react-checksum="197727696">Hello SSR!</h1>')
+    expect(css).toEqual(format(`


@mxstbr would you guys be open to switching these to snapshot tests? I think it might simplify things a bit.

Let's open an issue/another PR for that?

Will do. I added some new tests for previously uncovered code which use snaps but I'll do a separate PR to convert existing tests where it makes sense.

quantizor · 2017-07-22T17:06:38Z

Not really sure why rollup is failing. The error seems to indicate the JSX isn't being transformed?

quantizor · 2017-07-22T17:16:43Z

src/models/ServerStyleSheet.js

-      .map(key => this.components[key].css)
-      .join('')
+    const attrs = [
+      'type="text/css"',


I think style elements don't actually need this anymore.

Let's still keep it for completeness sake. We don't control what kind of HTML type this renders in. A bit redundant in 2017, but better than having people nitpick it.

mxstbr

LGTM, @jollife does this fix your issue?

mxstbr · 2017-07-22T18:02:06Z

src/test/ssr.test.js

+    const css = format(sheet.getStyleTags())
+
+    expect(html).toEqual('<h1 class="sc-a b" data-reactroot="" data-reactid="1" data-react-checksum="197727696">Hello SSR!</h1>')
+    expect(css).toEqual(format(`


Let's open an issue/another PR for that?

kitten

Some minor nits.

But another small thing; We'll have to check that using the global keyword is fine when it runs through rollup, and if it does, we have to check whether it causes any issues when it's not bundled using webpack. i.e. do we have to add a typeof check as well instead. Any best practices that we can refer to instead of blindly testing all these cases, as I'm a little unsure about some of them?

kitten · 2017-07-22T22:22:35Z

src/models/ServerStyleSheet.js


-    return `<style type="text/css" ${namesAttr} ${localAttr}>\n${css}\n</style>`
+    return `<style ${attrs.join(' ')}>${css}</style>`
  }

  toReactElement(key: string) {


The same props will need to be added to this method down here

kitten · 2017-07-22T22:23:27Z

src/models/ServerStyleSheet.js

+      `${LOCAL_ATTR}="${this.isLocal ? 'true' : 'false'}"`,
+    ]
+
+    /* eslint-disable no-underscore-dangle */


Can you instead just disable this globally for this file. Shouldn't be an issue.

Also what does the disabled eslint down below prevent from throwing an error? Seems like a bad idea to just disable everything without a comment.

It was complaining about using a comma and I guess there's an eslint rule that all arrow functions have to be one-liners without curlies?

Ok, in that case, just apply the suggestion below and it should become unnecessary ;)

kitten · 2017-07-22T22:24:50Z

src/models/ServerStyleSheet.js

+
+    /* eslint-disable */
+    const css = Object.keys(this.components).reduce((styles, key) => {
+      return (styles += this.components[key].css), styles


you can just return styles + this.components[key].css and even turn it into an arrow functions. Gets rid of the mutation and is easier to read.

I actually ended up making this into a reusable function since the same thing happens in toReactElement

quantizor · 2017-07-22T22:35:40Z

Looking into the rollup thing - might need to add this plugin: https://www.npmjs.com/package/rollup-plugin-node-globals

quantizor · 2017-07-22T22:49:13Z

Okay I found a way around it, using this seems to work without modifying rollup:

if (typeof __webpack_nonce__ !== 'undefined') {
    attrs.push(`nonce="${__webpack_nonce__}"`)
}

Basically just letting it implicitly refer to global scope

quantizor · 2017-07-22T22:57:43Z

@philpl addressed your comments

quantizor · 2017-07-22T23:14:49Z

~~Nvm the lack of global keyword seems to only work in node 8 - looking for other solutions...~~ fixed - had to do with the rollup commonjs plugin, see 00a9e54

Doesn't look like there were any output verification tests.

kitten · 2017-07-23T01:21:05Z

src/test/ssr.test.js

+
+    expect(elements).toHaveLength(2);
+
+    expect(elements[0].props).toMatchSnapshot()


I have yet another small nit here, sorry 😉

This test is basically the same as the above one, but we care about the nonce being present and set to
the global above. So we should probably just check for this directly instead of duplicating the above snapshots and having to check manually 😄

kitten · 2017-07-23T01:22:35Z

@probablyup Awesome work! I've only got one small nit left (see above comment)

quantizor · 2017-07-23T01:23:37Z

This might just be me, but I always test the base state and modifications separately. Easier to track individual regressions.

…

On Sat, Jul 22, 2017 at 9:22 PM Phil Plückthun ***@***.***> wrote: @probablyup <https://github.com/probablyup> Awesome work! I've only got one small nit left (see above comment) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiy1k-e1amAGFgXgtH-cEM6ZJlVrjEJks5sQqBcgaJpZM4OgOUZ> .

kitten · 2017-07-23T01:31:05Z

@probablyup that's a good default testing strategy, but in this case we don't care about anything but the nonce there. We're only testing:

Premise: A nonce is set as a global
Assumption: A nonce attr/prop is present

Nothing about the rest of the output. The two concerns don't overlap :)

quantizor · 2017-07-23T16:58:52Z

@philpl done

johannesnagl · 2017-07-23T18:11:38Z

@mxstbr sorry for Not giving any feedback. But my github handle is unfortunately Not @jollife.

Will investigate this Next week!

mxstbr · 2017-07-23T18:14:04Z

Oh no worries, please try it out and let us know if it works!

johannesnagl · 2017-07-24T11:30:41Z

Hi guys,

I tested this together with @stnwk and we found out the following:

Implementation is working
Nevertheless, the implementation is not using the correct __webpack_nonce__.

Clarification:

You're looking for window.__webpack_nonce__ or global.__webpack_nonce__. In our tests, when setting these values, the code is working fine. Although, WebPack itself is using the magic variable __webpack_nonce__, which is not a window or global variable.

See here:
https://github.com/webpack/webpack/blob/ab2270263e9af5b0dd83e454c20ab3aa1797424a/lib/APIPlugin.js#L26

It's getting replaced as a simple string.

So, the correct implementation would be the following:

    if (typeof __webpack_nonce__ !== 'undefined' && __webpack_nonce__) {
      this.el.setAttribute('nonce', __webpack_nonce__)
    }

We still have a problem with injectGlobal, where it's not being correctly set, but we haven't yet fully understand if it's a problem on our side or within styled-components.

stnwk · 2017-07-24T12:05:20Z

For the injectGlobal issue I think the following might be the reason:

In injectGlobal.js#L14 we are calling the function
inject(componentId, false, stringifyRules(rules)) on StyleSheet.js#L84 - which actually takes a forth and a fifth parameter (hash and name). But we are not passing either one of them.

In the case of being on the client-side, e.g. using BrowserStyleSheet this means the following function is called: BrowserStyleSheet.js#L61.

In this function we check wether the name parameter was passed before setting the nonce attribute, which isn't set when calling injectGlobal, because we are not passing it.

I'm not sure wether the check for the name parameter is on purpose there,
but that's the culprit I think.

Would also suggest to add a test for this specific case using injectGlobal.

Cheers :)

kitten · 2017-07-24T12:19:42Z

@probablyup are you open to creating a new PR with the above changes? :)

Fixing injectGlobal should be corrected by moving the nonce setAttribute logic out of the surrounding if-name clause, right?

quantizor · 2017-07-24T13:10:35Z

Ironically I had it that way for a while haha. I'm not sure how we are going to unit test a magic variable though since it's not actually in any scope.

…

On Mon, Jul 24, 2017 at 8:19 AM Phil Plückthun ***@***.***> wrote: @probablyup <https://github.com/probablyup> are you open to creating a new PR with the above changes? :) Fixing injectGlobal should be corrected by moving the nonce setAttribute logic out of the surrounding if-name clause, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiy1lNva7-Js9FHqtbBl3cWqT7igni1ks5sRIvfgaJpZM4OgOUZ> .

stnwk · 2017-07-24T13:16:29Z

Maybe taking a look at how webpack tests magic variables helps? @probablyup

quantizor · 2017-07-24T13:17:39Z

Yeah or making a util that returns it and mocking the util. That's probably the most straightforward.

…

On Mon, Jul 24, 2017 at 9:16 AM Stefan ***@***.***> wrote: Maybe taking a look at how webpack tests magic variables <https://github.com/webpack/webpack/blob/master/test/cases/nonce/set-nonce/index.js>helps? @probablyup <https://github.com/probablyup> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiy1id_asthBu_8AdCoNyihKxCDJNH-ks5sRJkugaJpZM4OgOUZ> .

quantizor commented Jul 22, 2017

View reviewed changes

quantizor force-pushed the es-implement-nonce branch from 4951223 to bfc2868 Compare July 22, 2017 17:14

quantizor commented Jul 22, 2017

View reviewed changes

mxstbr approved these changes Jul 22, 2017

View reviewed changes

kitten requested changes Jul 22, 2017

View reviewed changes

quantizor force-pushed the es-implement-nonce branch from bfc2868 to 97c7cc1 Compare July 22, 2017 22:56

quantizor force-pushed the es-implement-nonce branch from 00a9e54 to cfdd946 Compare July 22, 2017 23:43

quantizor added 2 commits July 22, 2017 19:44

feat: __webpack_nonce__ support in SSR + Browser contexts

4833ecc

test: add tests for sheet.getStyleElement()

1eea50e

Doesn't look like there were any output verification tests.

quantizor force-pushed the es-implement-nonce branch from cfdd946 to 1eea50e Compare July 22, 2017 23:44

kitten reviewed Jul 23, 2017

View reviewed changes

kitten approved these changes Jul 23, 2017

View reviewed changes

quantizor added 2 commits July 23, 2017 12:57

test: simplify the SSR+nonce getStyleElement() test

91388b8

Merge branch 'master' into es-implement-nonce

f46b600

kitten merged commit 96e7663 into styled-components:master Jul 23, 2017

arahansen mentioned this pull request Dec 14, 2017

feat(CSP): allow nonce cxs-css/cxs#97

Open

bodhi mentioned this pull request Jun 13, 2018

Setting nonce per-request with streaming SSR #1802

Closed

plondon mentioned this pull request Aug 16, 2018

CSP ReactTooltip/react-tooltip#340

Closed

avindra mentioned this pull request May 11, 2020

link to styled components issue theKashey/get-nonce#1

Merged

bwhitty mentioned this pull request Mar 29, 2022

[Feature Request] Support a nonce argument directly in 'getStyleElement' for SSR #3468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: __webpack_nonce__ support in SSR + Browser contexts #1022

feat: __webpack_nonce__ support in SSR + Browser contexts #1022

quantizor commented Jul 22, 2017

quantizor Jul 22, 2017

quantizor Jul 22, 2017 •

edited

Loading

mxstbr Jul 22, 2017

quantizor Jul 22, 2017

quantizor commented Jul 22, 2017

quantizor Jul 22, 2017 •

edited

Loading

kitten Jul 22, 2017

mxstbr left a comment

mxstbr Jul 22, 2017

kitten left a comment •

edited

Loading

kitten Jul 22, 2017

kitten Jul 22, 2017

quantizor Jul 22, 2017

kitten Jul 22, 2017

kitten Jul 22, 2017

quantizor Jul 22, 2017

quantizor commented Jul 22, 2017

quantizor commented Jul 22, 2017 •

edited

Loading

quantizor commented Jul 22, 2017

quantizor commented Jul 22, 2017 •

edited

Loading

kitten Jul 23, 2017

kitten commented Jul 23, 2017

quantizor commented Jul 23, 2017 via email

kitten commented Jul 23, 2017

quantizor commented Jul 23, 2017

johannesnagl commented Jul 23, 2017

mxstbr commented Jul 23, 2017

johannesnagl commented Jul 24, 2017

stnwk commented Jul 24, 2017

kitten commented Jul 24, 2017

quantizor commented Jul 24, 2017 via email

stnwk commented Jul 24, 2017

quantizor commented Jul 24, 2017 via email


		expect(elements).toHaveLength(2);

		expect(elements[0].props).toMatchSnapshot()

feat: __webpack_nonce__ support in SSR + Browser contexts #1022

feat: __webpack_nonce__ support in SSR + Browser contexts #1022

Conversation

quantizor commented Jul 22, 2017

Choose a reason for hiding this comment

quantizor Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantizor commented Jul 22, 2017

quantizor Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxstbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitten left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantizor commented Jul 22, 2017

quantizor commented Jul 22, 2017 • edited Loading

quantizor commented Jul 22, 2017

quantizor commented Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

kitten commented Jul 23, 2017

quantizor commented Jul 23, 2017 via email

kitten commented Jul 23, 2017

quantizor commented Jul 23, 2017

johannesnagl commented Jul 23, 2017

mxstbr commented Jul 23, 2017

johannesnagl commented Jul 24, 2017

stnwk commented Jul 24, 2017

kitten commented Jul 24, 2017

quantizor commented Jul 24, 2017 via email

stnwk commented Jul 24, 2017

quantizor commented Jul 24, 2017 via email

quantizor Jul 22, 2017 •

edited

Loading

quantizor Jul 22, 2017 •

edited

Loading

kitten left a comment •

edited

Loading

quantizor commented Jul 22, 2017 •

edited

Loading

quantizor commented Jul 22, 2017 •

edited

Loading