Skip to content
This repository has been archived by the owner on Feb 9, 2023. It is now read-only.

Fixed incorrect parsing/stringifying for nested tagged template literals #17

Merged
merged 3 commits into from
Apr 3, 2020

Conversation

ota-meshi
Copy link
Member

Which issue, if any, is this issue related to?

refs gucong3000#53

Is there anything in the PR that needs further explanation?

I solved the nested template problem by making the nested template literal a child node.

@ota-meshi ota-meshi force-pushed the issue-orig-53 branch 2 times, most recently from cb2939a to 9d93fcd Compare April 1, 2020 14:31
@jeddy3 jeddy3 mentioned this pull request Apr 2, 2020
6 tasks
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

That's a tremendous work! Thank you for fixing this!

${(props) => css`
color: #b02d00;
`}
${(props2) => css`
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that in every test there is css function before nested template literal. Will this work without it? E. g.

import styled, { css } from 'styled-components';
const Message = styled.p`
	padding: 10px;
	${(props) => `
		color: #b02d00;
	`}
	${(props2) => `
		border-color: red;
	`}
`;

Could you add a test for this, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @hudochenkov.

I have added your requested test case.
https://github.com/stylelint/postcss-css-in-js/pull/17/files#diff-afcadaa7a1aaa2ea2d86507936d47fba

But this case doesn't parse the template literal inside as CSS. The inside template literal is converted to a literal node with no child nodes.

TaggedTemplateExpression is required to parse as CSS.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

I think it's totally ok, because it impossible to know what inside template literal without the tag.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Amazing work!

@jeddy3 jeddy3 mentioned this pull request Apr 3, 2020
@jeddy3
Copy link
Member

jeddy3 commented Apr 3, 2020

Excellent stuff. Am I right in thinking that this will fix stylelint/stylelint#4119, and therefore we'll need to remove the corresponding workaround?

Once #19 is merged, let's rebase this pull request.

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Fantastic.

@jeddy3 jeddy3 merged commit 2f174f3 into stylelint:master Apr 3, 2020
@ota-meshi
Copy link
Member Author

In my opinion, there is no need for a workaround, as it fixes the problem that toString breaks. However, changes to the AST might cause unexpected and incorrect reports.

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

Successfully merging this pull request may close these issues.

3 participants