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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing test and proposed fix for css({'data-css-nil': ''}) #373

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

Conversation

penx
Copy link

@penx penx commented Mar 21, 2018

I can't see any documentation or explanation in the code as to what data-css-nil is for, other than:

glamor/src/index.js

Lines 430 to 432 in db533d7

let nullrule = {
// 'data-css-nil': ''
}

...and some perhaps related discussion in issue #303.

However, Glamorous, under certain conditions, will call css({'data-css-nil': ''}), which causes Glamor to throw the following error:

Error: [glamor] an unexpected rule cache miss occurred. This is probably a sign of multiple glamor instances in your app. See #79

See:

paypal/glamorous#396
paypal/glamorous#397

I am not sure if this bug is due to Glamorous sending something through that it shouldn't, or Glamor not handling this correctly, so have created failing tests for both scenarios.

I can't find references to data-css-nil in Glamorous' code base but can continue to debug to try find out what's causing this. However if anyone has any insight on this that may save a lot of code reading then please let me know 馃檪

@penx penx changed the title Failing test for glamorous issue #396 Failing test for data-css-nil Mar 21, 2018
@kentcdodds
Copy link
Contributor

Thanks for opening this! This is a great first step to figuring out what's up!

I personally don't have time to help with this, and I don't understand the inner-workings of glamor super-well, but hopefully someone can help.

@penx
Copy link
Author

penx commented Mar 21, 2018

I was starting to understand it earlier. Will take another look tomorrow

@tikotzky
Copy link

Seems like it was originally added to fix #115

@penx penx changed the title Failing test for data-css-nil Failing test and possible fix for css({'data-css-nil': ''}) Mar 22, 2018
@penx penx changed the title Failing test and possible fix for css({'data-css-nil': ''}) Failing test and proposed fix for css({'data-css-nil': ''}) Mar 22, 2018
@penx
Copy link
Author

penx commented Mar 22, 2018

I've added a proposed fix to this PR that makes build ignore rules where the ID is 'nil'

@JakeGinnivan
Copy link

Another scenario which would be handy to fix is glamorous(MyComponent)({}) throws the same error.

@penx
Copy link
Author

penx commented Apr 10, 2018

Travis failure is unrelated:

22 03 2018 08:53:12.786:ERROR [launcher]: Cannot start Chrome
[3353:3353:0322/085312.638592:FATAL:setuid_sandbox_host.cc(157)] The SUID sandbox helper binary was found, but is not configured correctly. Rather than run without sandboxing I'm aborting now. You need to make sure that /opt/google/chrome/chrome-sandbox is owned by root and has mode 4755.

@penx penx mentioned this pull request Apr 18, 2018
@frontendphil
Copy link

Hey @penx, since we at Signavio believe in this library we want to continue developing it further. Therefore, we've forked it and will release it as signavio-glamor. It would be awesome if you could direct your PR against our fork so that we can include it!

https://github.com/signavio/glamor

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.

None yet

5 participants