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

The style is .css-4oa7uv-Header.Space rather than .css-4oa7uv-Header\.Space #2638

Closed
PeterlitsZo opened this issue Feb 12, 2022 · 14 comments
Closed
Assignees
Labels

Comments

@PeterlitsZo
Copy link

Current behavior:

It make the className as .css-4oa7uv-Header.Space, and it also make a .css-4oa7uv-Header.Space style rules. But it should be .css-4oa7uv-Header.Space rule, it should be .css-4oa7uv-Header\.Space rule.

Here is my code:

// Header.Space = function HeaderSpace({ children }: HeaderSpaceProps) {
Header.Space = ({ children }: HeaderSpaceProps) => {
  return (
    <div
      sx={{
        py: mRV({ _: '6rem', sm: '8rem', md: '8rem' }),
        display: 'flex',
        flexDirection: 'column',
        alignItems: 'center',
        gap: '2rem',
      }}
    >
      {children}
    </div>
  )
};

When I export Header, I find that Header.Space component doesn't work well.
I check what happens. Well, there is a .css-4oa7uv-Header.Space style rule!
But the right name should be .css-4oa7uv-Header\.Space. It forget \ sign!
I read the emtion source file. I find it will get the label name from function call stack if the NODE_ENV is development.
I try to rewrite the jest's test file. But I cannot make it again in test file...
Could you help me? And do you need a runable demo? If you pointer out how to let it show again in test, I think I can help you with a PR.

Expected behavior:

The style rule should be for .css-4oa7uv-Header\.Space.

Environment information:

  • react version: ^17.0.2
  • @emotion/react version: ^11.7.1
@Andarist
Copy link
Member

Please always try to share a repro case in a runnable form - either by providing a git repository to clone or a codesandbox. OSS maintainers usually can't afford the time to set up the repro, even if exact steps are given.

@PeterlitsZo
Copy link
Author

OK.

@PeterlitsZo
Copy link
Author

@Andarist
Copy link
Member

I see this in the devtools:
Screenshot 2022-02-13 at 21 02 15
and it renders OK:
Screenshot 2022-02-13 at 21 03 50

In what browser do you experience this problem?

@Andarist
Copy link
Member

Ok, I've found the issue in FireFox. We are indeed matching App.Space with this regex: https://regex101.com/r/TF1HFL/1 and we fail to sanitize this here:

const sanitizeIdentifier = (identifier: string) =>
identifier.replace(/\$/g, '-')

We either should sanitize this in the linked function or match a different string with a regexp, cc @srmagura

@PeterlitsZo
Copy link
Author

Or change the build css style name from .css-xxxxxx-App.Space to .css-xxxxxx-App\.Space? By the way, thank you for your reply.

@Andarist
Copy link
Member

Or change the build css style name from .css-xxxxxx-App.Space to .css-xxxxxx-App.Space?

This is unlikely to happen. I don't think that the dot here is worth preserving - we also need to consider what is generated for this setup during SSR because this has to match with the client.

@srmagura
Copy link
Member

srmagura commented Feb 14, 2022 via email

@PeterlitsZo
Copy link
Author

Or change the build css style name from .css-xxxxxx-App.Space to .css-xxxxxx-App.Space?

This is unlikely to happen. I don't think that the dot here is worth preserving - we also need to consider what is generated for this setup during SSR because this has to match with the client.

OK. The chrome will just using Space, we would not use Header.Space in firefox.

@srmagura srmagura self-assigned this Feb 18, 2022
@srmagura
Copy link
Member

This case was fixed in #2615, which was released as part of @emotion/react 11.8.0. I upgraded the minimal reproduction to the latest @emotion/react and the class name became css-xxxxxx-Space which is what I expected.

@PeterlitsZo
Copy link
Author

Thanks for your great works!!!

By the way, I cannot get your test code, could you please... help me to get it?

expect(getLabelFromStackTrace(stackTrace)).toBeUndefined()

After read source code, in my mind that the function getLabelFromStackTrace will return a name which its first character is uppercase. But... why in this case it be undefined?

Sorry that my question is stupid...

@srmagura
Copy link
Member

@PeterlitsZo What line in the source file did that come from?

getLabelFromStackTrace returns undefined in two cases:

  • Class components
  • Safari does not include the component name in the stack due to Proper Tail Calls

@PeterlitsZo
Copy link
Author

PeterlitsZo commented Feb 20, 2022

@srmagura Thanks for your kindly and swift reply. source file here:

https://github.com/emotion-js/emotion/pull/2615/files#diff-2230d727808a5ee4f8d53c68f46f2d5093d885d7865db7e78be0d87cb6841a82R630

I find this line: MyComponent$9</MyComponent$9.prototype.render..., and this comment tell me that if the first character is uppercase, it is the name should return.

So I except it should return 'MyComponent$9' rather than undefined

@srmagura
Copy link
Member

That stack trace comes from a class component, so getLabelFromStackTrace is expected to return undefined. The regular expressions in getFunctionNameFromStackTraceLine will not match MyComponent$9</MyComponent$9.prototype.render because of the special characters.

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

No branches or pull requests

3 participants