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

Reinstate and document workaround feature #4592

Merged
merged 9 commits into from Feb 14, 2020
Merged

Reinstate and document workaround feature #4592

merged 9 commits into from Feb 14, 2020

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Feb 10, 2020

(If time is short, please review this pull request over #4591 so that we can address #3514.)

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

Follow on from #4573

Is there anything in the PR that needs further explanation?

This pull request is also part of the stream of work to better surface syntaxes so that we can muster support for them. After we merge this pull request, I plan to post comments on the issues mentioned in #4574 pointing to this new feature.

--

I'm not sure how it happened, but buried at the bottom of #4588 were some changes that reverted the actual workaround feature added in #4573. I've recovered those changes in this pull request.

@jeddy3 jeddy3 changed the title Document syntaxes and workaround feature Reinstate and document workaround feature Feb 13, 2020
lib/lintSource.js Outdated Show resolved Hide resolved
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Looks fine

docs/developer-guide/syntaxes.md Outdated Show resolved Hide resolved
docs/developer-guide/syntaxes.md Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

I left some notes about TypeScript definitions. Perhaps we will handle them later.
(They are enough in this PR!)


interface Input {
css?: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

[note] This css property is missing the official PostCSS type definitions:

https://github.com/postcss/postcss/blob/ee267d5f6406e9a9ca88be4191ffb6c8b648dc15/lib/input.es6#L25-L34

interface NodeSource {
lang?: string;

}
Copy link
Member

Choose a reason for hiding this comment

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

[note] This lang property seems an extension by postcss-syntax. I cannot find it in the PostCSS core repository.

https://github.com/gucong3000/postcss-syntax/blob/0bca5a408f12891a622fd6149d8f23d3bd98df86/syntax.js#L17

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know what the best way of handling this is?

Copy link
Member

Choose a reason for hiding this comment

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

No, I'm not sure now. 😣
This solution seems best for us now.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

jeddy3 and others added 8 commits February 13, 2020 21:27
Co-Authored-By: Stephen Edgar <stephen@netweb.com.au>
Co-Authored-By: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-Authored-By: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Nice! 👍

@jeddy3 jeddy3 merged commit 36e2dbb into master Feb 14, 2020
@jeddy3 jeddy3 deleted the pull-4573 branch February 14, 2020 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants