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

Replace syntaxes with our forks #4685

Merged
merged 7 commits into from
Apr 7, 2020
Merged

Replace syntaxes with our forks #4685

merged 7 commits into from
Apr 7, 2020

Conversation

hudochenkov
Copy link
Member

@hudochenkov hudochenkov commented Apr 2, 2020

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

Closes #4645
Closes #4119

Is there anything in the PR that needs further explanation?

It doesn't work yet. First, we need to release @stylelint/postcss-css-in-js and @stylelint/postcss-markdown. Then we can test and make changes to this PR.

@hudochenkov hudochenkov added pr: needs revision pr: blocked is blocked by another issue or pr labels Apr 2, 2020
@alexander-akait alexander-akait self-requested a review April 3, 2020 10:37
@jeddy3 jeddy3 mentioned this pull request Apr 3, 2020
10 tasks
@jeddy3 jeddy3 removed the pr: blocked is blocked by another issue or pr label Apr 6, 2020
@jeddy3 jeddy3 force-pushed the stylelint-postcss-css-in-js branch from f5478f7 to 0cf46bc Compare April 6, 2020 12:01
@jeddy3
Copy link
Member

jeddy3 commented Apr 6, 2020

I've updated the pull request to use our forks.

The lint task is failing on the types check:

##[error]lib/getPostcssResult.js(97,19): error TS7016: Could not find a declaration file for module '@stylelint/postcss-css-in-js'. '/home/runner/work/stylelint/stylelint/node_modules/@stylelint/postcss-css-in-js/index.js' implicitly has an 'any' type.
  Try `npm install @types/stylelint__postcss-css-in-js` if it exists or add a new declaration (.d.ts) file containing `declare module '@stylelint/postcss-css-in-js';`
##[error]lib/getPostcssResult.js(98,24): error TS7016: Could not find a declaration file for module '@stylelint/postcss-markdown'. '/home/runner/work/stylelint/stylelint/node_modules/@stylelint/postcss-markdown/index.js' implicitly has an 'any' type.
  Try `npm install @types/stylelint__postcss-markdown` if it exists or add a new declaration (.d.ts) file containing `declare module '@stylelint/postcss-markdown';`

And 1 test related to #4119 is, understandably, failing:

Summary of all failing tests
FAIL system-tests/fix/fix.test.js
  ● fix › doesn't fix with nested template literals

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      199 | 			})
      200 | 			.then((result) => {
    > 201 | 				expect(result.errored).toBe(true);
          | 				                       ^
      202 | 				expect(result.output).toBe(code);
      203 | 			});
      204 | 	});

      at stylelint.lint.then.result (system-tests/fix/fix.test.js:201:28)


Test Suites: 1 failed, 305 passed, 306 total
Tests:       1 failed, 4 skipped, 15121 passed, 15126 total
Snapshots:   9 passed, 9 total

I suspect because the workaround to ignore nested template literals relies on the AST that has changed (and is now hopefully fixed). If the underlying bug in the parser is fixed, we should remove the workaround (and its corresponding test).

I don't have any time left today, so feel free to jump into this pull request if anyone has time.

@jeddy3
Copy link
Member

jeddy3 commented Apr 7, 2020

Thanks for updating the pull request. I've confirmed locally that #4119 is resolved and that the source file is no longer corrupted when using --fix. However, the indentation rule has false positives:

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

Produces:

jeddy3@jeddy3 stylelint-test % npx stylelint "sources/**/fix.js"      

sources/js/fix.js
 5:5  ✖  Expected indentation of 0 spaces   indentation
 6:2  ✖  Expected indentation of 4 spaces   indentation

The indentation rule is one of the few rules that deal with non-standard syntax rather than ignoring it via the isStandardSyntax* utils. Do we want to fix indentation as part of this pull request, or shall I create a follow-up issue and we release?

I think we should use this release to drum up support for the CSS-in-JS parser by announcing in the release tweet that we're now using our own fork of the CSS-in-JS and we need help to fix bugs with it. We can add the indentation issue above to that list.

@hudochenkov
Copy link
Member Author

hudochenkov commented Apr 7, 2020

I think we should create a separate issue, and preferably not release a new version until this issue is fixed. Because it's a breaking issue, and we know about it.

Thank you for finding it!

@jeddy3
Copy link
Member

jeddy3 commented Apr 7, 2020

Agreed. New issue created.

This pull request looks good to me. I think we can merge.

@ota-meshi
Copy link
Member

I made a pull request for this branch. #4695

@jeddy3 jeddy3 merged commit e1c8d2d into master Apr 7, 2020
@jeddy3 jeddy3 deleted the stylelint-postcss-css-in-js branch April 7, 2020 16:54
@jeddy3
Copy link
Member

jeddy3 commented Apr 7, 2020

  • Fixed: babel configuration conflict when using TypeScript (postcss-css-in-js/#2).
  • Fixed: autofix for nested tagged template literals (#4119).

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.

Configure postcss-syntax to use forked syntaxes Fix autofix for nested tagged template literals
4 participants