Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support Strict CSP with a nonce #482
Support Strict CSP with a nonce #482
Changes from 3 commits
f15dc2d
bb98185
fe01906
278e5d5
6f5f14f
368a6e4
bb588e9
195bcf1
0cdda13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be
js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can use
this._isBrowser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like changing this might break my test 😟 because there isn't any browser testing implemented in this repo☹️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably in your test you can do
global.window = true
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah no you can pass
isBrowser: true
tomakeRegistry
https://github.com/zeit/styled-jsx/blob/master/test/stylesheet-registry.js#L8-L12There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@giuseppeg Will this not cause unintended side effects because we are telling the application it is running in a browser and it's not really? If somewhere else in the application it checks
isBrowser
and if I set it to true, it might try to execute some other type of browser only script.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an isolated unit test so it is perfectly fine. If you are talking about
src/lib/stylesheet.js
instead that's fine too since you are checking thattypeof document === 'object'
is defined i.e. you are in a browser environment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was right about the unintended side effects. It's possible that
isBrowser == true
andtypeof document === 'undefined'
, which will create an error.Making the change you requested, allows other tests to break code when it sets isBrowser to true, even when it's not technically in a browser. I feel like this is an issue of it's own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should behave the same when isBrowser is true. If there are failing tests this is a good thing and we should look into this issue. In a real browser
typeof document === 'object'
will be the same as usingthis._isBrowser
because the latter is justtypeof window !== 'undefined;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want me to break the tests, I will, I just don't have time to fix them myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= undefined
is redundant, you can remove it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make the arguments an object? This will allow us to add more options in the future.