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

[test] Throw on console.(error|warn) outside of test #22907

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 6, 2020

Should be reviewed without whitespace diff.

  1. Get rid of "You have illegal escape sequence in your template literal" console message
    Leaving a FIXME
  2. console.(warn|error) outside of if it() now throws
    I don't know anymore if this was intentional or not.
  3. Move work in experimentalStyled.test into before
    Tests should be side-effect free i.e. don't execute any code.
    If they do you'll have a hard time locating the issue.
    It also slows down the overall test suite when doing --grep because test runners have to evaluate the full test suite. But this is more of a "death-by-thousand-cuts" scenario

@eps1lon eps1lon added the test label Oct 6, 2020
Comment on lines +91 to +108
expect(() => {
Test = styled(
'div',
{ shouldForwardProp: (prop) => prop !== 'variant' && prop !== 'size' },
{ muiName: 'MuiTest', overridesResolver: testOverridesResolver },
)`
width: 200px;
height: 300px;
`;
}).toErrorDev([
'You have illegal escape sequence in your template literal',
'You have illegal escape sequence in your template literal',
]);
Copy link
Member Author

Choose a reason for hiding this comment

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

@mnajdova Any idea why this is happening?

Copy link
Member

Choose a reason for hiding this comment

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

Nice! I was wondering where this warning was coming from!

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why it is happening. Let's replace the string template with object:

({
  width: '200px',
  height: '300px',
})  

and it will go away. I will try to repro it on a simple example and report it.

Copy link
Member

Choose a reason for hiding this comment

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

Update, even just adding round bracket around it solves the issue 🤷 :

(`
  width: 200px;
  height: 300px;
`)

Copy link
Member

@mnajdova mnajdova Oct 13, 2020

Choose a reason for hiding this comment

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

https://codesandbox.io/s/objective-vaughan-6r3u0?file=/src/App.js

Found the issue, seems like the destructuring on line 31 causes the warning:

return defaultStyledResolver(...stylesWithDefaultTheme);

If I remove it the warning is gone.

return defaultStyledResolver(stylesWithDefaultTheme);

However, styled-components was not working without the destructuring. - https://codesandbox.io/s/illegal-escape-warning-styled-components-9ug0i?file=/src/App.js so I would propose to not change anything at this moment. If in the next version of emotion the warning is fixed, we can update it. Does that makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like this is what the adapter packages were made for.

Copy link
Member

Choose a reason for hiding this comment

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

True, I will try to update them today and test again

Copy link
Member Author

Choose a reason for hiding this comment

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

Marked as FIXME. Needs to be fixed before release. For now we keep the expectation so that we don't add noise to the console output.

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 6, 2020

No bundle size changes comparing fcee9db...d137f10

Generated by 🚫 dangerJS against d137f10

@eps1lon eps1lon marked this pull request as ready for review October 15, 2020 08:01
@eps1lon eps1lon merged commit 777c1a1 into mui:next Oct 15, 2020
@eps1lon eps1lon deleted the fix/escape-log branch October 15, 2020 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants