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

Always infer syntax for known file formats, even with processors. #5132

Closed
wants to merge 1 commit into from
Closed

Always infer syntax for known file formats, even with processors. #5132

wants to merge 1 commit into from

Conversation

yaymukund
Copy link

@yaymukund yaymukund commented Feb 6, 2021

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

closes #5117

Is there anything in the PR that needs further explanation?

Unfortunately, I but couldn't figure out a minimal reproduction so I couldn't add a test. I suspect it might only break in our case because of some interaction with processors: ['stylelint-processor-styled-components']. However, I can confirm:

  • An example of a scss file in our repository that errors without this code, but is fixed with this code:
    .foo {
      // a nested comment
      &--bar { color: red; }
    }
  • The code in this PR fixes the issue in our repository.
  • The tests seem to pass, so in theory this shouldn't break anything.

I understand if you still don't want to merge this but i've spent a couple hours on this and don't have time to dive any further. (We actually ended up passing -s scss to work around this issue, so it's no longer blocking us.)

} else if (!(options.codeProcessors && options.codeProcessors.length)) {
} else if (
!(options.codeProcessors && options.codeProcessors.length) ||
(options.filePath && /\.(scss|sass|less)$/.test(options.filePath))
Copy link
Member

Choose a reason for hiding this comment

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

I believe it defeats the purpose of postcss-syntax, and removes support for many supported extensions, e. g. .js, .ts, .html, .vue.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I understand. I'm only adding an || branch so if a file's syntax was previously inferred, then it should continue being inferred. This infers the syntax for more filetypes. Are you suggesting I add those extensions to this list?

For context, this is a copy-paste of the code as it was before it was accidentally removed in https://github.com/stylelint/stylelint/pull/4729/files#diff-5383a0a864da5a5ad24b4bae8f44c5574601bd4d145999b1ddc211d3900dd68bL89

Thanks for taking a look :)

Copy link
Member

@jeddy3 jeddy3 Feb 6, 2021

Choose a reason for hiding this comment

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

I understand if you still don't want to merge this but i've spent a couple hours on this

@yaymukund Thank you for taking the time to dig into this. The syntax hullabaloo in stylelint is complex. I'm personally looking forward to the possibility of removing it in favour of explicit-configuration using a overrides property. #5126 is the first step towards that.

Anyhow, in the meantime, would you mind removing the else if in favour of simple else, i.e. autosyntax is always used regardless of if a processor is configured in the configuration object?:

} else { 
	const autoSyntax = require('postcss-syntax');

	// TODO: investigate why lazy import HTML syntax causes
	// JS files with the word "html" to throw TypeError
	// https://github.com/stylelint/stylelint/issues/4793
	const { html, ...rest } = syntaxes;

	syntax = autoSyntax({
		css: cssSyntax(stylelint),
		jsx: syntaxes['css-in-js'],
		...rest,
	});
}

And seeing if that works in your repository.

(We actually ended up passing -s scss to work around this issue, so it's no longer blocking us.)

Ha, yes, that'll do it :)

Copy link
Author

Choose a reason for hiding this comment

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

It looks like that condition has existed for as long as postcss-safe-parser:

https://github.com/stylelint/stylelint/pull/3261/files#diff-5383a0a864da5a5ad24b4bae8f44c5574601bd4d145999b1ddc211d3900dd68bR89

But changing it to else seems to work for our repo, and the tests here pass, so this should be GTM if all that checks out for you :)

Re: syntax configuration: that makes sense. I would be in favor of an explicit opt-in API, if only so we can more easily reason about what's happening.

Copy link
Member

@jeddy3 jeddy3 Feb 7, 2021

Choose a reason for hiding this comment

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

Thanks for making the change and testing it with your repo!

@hudochenkov what do you think, can we merge this or would you rather hold off while you're looking into what the future of syntaxes could be?

@yaymukund yaymukund requested a review from jeddy3 February 6, 2021 15:55
@jeddy3
Copy link
Member

jeddy3 commented May 18, 2021

Thanks for the pull request.

We're going to take a different direction with 14.0.0.

Closed by #5297.

@jeddy3 jeddy3 closed this May 18, 2021
@yaymukund yaymukund deleted the infer-syntax-for-known-file-extensions branch May 18, 2021 15:50
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.

Fix syntax inference with processors
3 participants