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

Update to postcss-less@3 #3687

Merged
merged 8 commits into from Oct 16, 2018

Conversation

6 participants
@jwilsson
Member

jwilsson commented Sep 22, 2018

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

Closes #3686

Is there anything in the PR that needs further explanation?

The biggest change is probably the removal of Import nodes from postcss-less, they will now always be a usual AtRule node. I've removed tests and doc notes around that.

Otherwise, it should be pretty straightforward. I've left some inline notes too.

check(atRule, atRule.params, atRuleParamIndex)
);
root.walkAtRules(atRule => {
if (!/^media$/i.test(atRule.name) && !atRule.variable) {

This comment has been minimized.

@jwilsson

jwilsson Sep 22, 2018

Member

There were a bunch of tests for @variable syntax, hence this change even though it's non-standard syntax. Perhaps those tests should be updated instead?

@jwilsson

jwilsson Sep 22, 2018

Member

There were a bunch of tests for @variable syntax, hence this change even though it's non-standard syntax. Perhaps those tests should be updated instead?

check(atRule, atRule.params, atRuleParamIndex)
);
root.walkAtRules(atRule => {
if (!/^media$/i.test(atRule.name) && !atRule.variable) {

This comment has been minimized.

@jwilsson

jwilsson Sep 22, 2018

Member

Same as above, there were a bunch of tests for @variable syntax, hence this change even though it's non-standard syntax. Perhaps those tests should be updated instead?

@jwilsson

jwilsson Sep 22, 2018

Member

Same as above, there were a bunch of tests for @variable syntax, hence this change even though it's non-standard syntax. Perhaps those tests should be updated instead?

@@ -7,17 +7,21 @@
* @param {atRule} postcss at-rule node
* @return {boolean} If `true`, the declaration is standard
*/
module.exports = function(atRule /*: postcss$atRule*/) /*: boolean*/ {
module.exports = function(atRule /*: Object*/) /*: boolean*/ {

This comment has been minimized.

@jwilsson

jwilsson Sep 22, 2018

Member

Perhaps someone better than me at Flow can help out with this.

@jwilsson

jwilsson Sep 22, 2018

Member

Perhaps someone better than me at Flow can help out with this.

This comment has been minimized.

@hudochenkov

hudochenkov Sep 22, 2018

Member

I have no clue about Flow, to be honest. Maybe new properties should be added to this lines?

declare class postcss$atRule extends postcss$node {
name: string;
params: string;
raws: {
before?: string,
after?: string,
afterName?: string
};
}

@hudochenkov

hudochenkov Sep 22, 2018

Member

I have no clue about Flow, to be honest. Maybe new properties should be added to this lines?

declare class postcss$atRule extends postcss$node {
name: string;
params: string;
raws: {
before?: string,
after?: string,
afterName?: string
};
}

This comment has been minimized.

@ntwb

ntwb Oct 9, 2018

Member

@CAYdenberg Any chance you've got a spare moment to check on this Flow issue please?

@ntwb

ntwb Oct 9, 2018

Member

@CAYdenberg Any chance you've got a spare moment to check on this Flow issue please?

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Sep 22, 2018

Contributor

postcss-less used PostCSS 5, so updating postcss-less will be very cool @ntwb

Contributor

ai commented Sep 22, 2018

postcss-less used PostCSS 5, so updating postcss-less will be very cool @ntwb

@hudochenkov

Thank you for picking this up!

no-extra-semicolons was changed. Maybe it needs new tests?

Please, add tests for isStandardSyntaxAtRule.

@@ -7,17 +7,21 @@
* @param {atRule} postcss at-rule node
* @return {boolean} If `true`, the declaration is standard
*/
module.exports = function(atRule /*: postcss$atRule*/) /*: boolean*/ {
module.exports = function(atRule /*: Object*/) /*: boolean*/ {

This comment has been minimized.

@hudochenkov

hudochenkov Sep 22, 2018

Member

I have no clue about Flow, to be honest. Maybe new properties should be added to this lines?

declare class postcss$atRule extends postcss$node {
name: string;
params: string;
raws: {
before?: string,
after?: string,
afterName?: string
};
}

@hudochenkov

hudochenkov Sep 22, 2018

Member

I have no clue about Flow, to be honest. Maybe new properties should be added to this lines?

declare class postcss$atRule extends postcss$node {
name: string;
params: string;
raws: {
before?: string,
after?: string,
afterName?: string
};
}

@jwilsson

This comment has been minimized.

Show comment
Hide comment
@jwilsson

jwilsson Sep 23, 2018

Member

@ hudochenkov Thanks for the review! I've added a new test for isStandardSyntaxAtRule.

no-extra-semicolons should be fine with the current tests, there are a few for mixins where the node type where changed from Rule to AtRule.

Member

jwilsson commented Sep 23, 2018

@ hudochenkov Thanks for the review! I've added a new test for isStandardSyntaxAtRule.

no-extra-semicolons should be fine with the current tests, there are a few for mixins where the node type where changed from Rule to AtRule.

@jeddy3

This comment has been minimized.

Show comment
Hide comment
@jeddy3

jeddy3 Oct 8, 2018

Member

Is there anything blocking this PR or can we get it in?

Member

jeddy3 commented Oct 8, 2018

Is there anything blocking this PR or can we get it in?

@ntwb

ntwb approved these changes Oct 9, 2018

@jeddy3

This comment has been minimized.

Show comment
Hide comment
@jeddy3

jeddy3 Oct 9, 2018

Member

@hudochenkov Have your concerns been addressed?

Member

jeddy3 commented Oct 9, 2018

@hudochenkov Have your concerns been addressed?

@jeddy3 jeddy3 referenced this pull request Oct 9, 2018

Closed

Failing appveyor test #3712

@jeddy3 jeddy3 removed the PR: needs tests label Oct 11, 2018

@jeddy3

This comment has been minimized.

Show comment
Hide comment
@jeddy3

jeddy3 Oct 15, 2018

Member

I rebased this on master, but node 10 (but not node 8) is failing on appveyor:

FAIL lib/__tests__/ignore.test.js
  ● using ignoreFiles with input files that would cause a postcss syntax error › no-syntax-error.css found
    expect(received).not.toBe(expected) // Object.is equality
    Expected: -1
    Received: -1
      307 | 
      308 |   it("no-syntax-error.css found", () => {
    > 309 |     expect(results[0].source.indexOf("no-syntax-error.css")).not.toBe(-1);
          |                                                                  ^
      310 |   });
      311 | 
      312 |   it("no-syntax-error.css linted", () => {
      at Object.toBe (lib/__tests__/ignore.test.js:309:66)

Anyone got an idea why?

Member

jeddy3 commented Oct 15, 2018

I rebased this on master, but node 10 (but not node 8) is failing on appveyor:

FAIL lib/__tests__/ignore.test.js
  ● using ignoreFiles with input files that would cause a postcss syntax error › no-syntax-error.css found
    expect(received).not.toBe(expected) // Object.is equality
    Expected: -1
    Received: -1
      307 | 
      308 |   it("no-syntax-error.css found", () => {
    > 309 |     expect(results[0].source.indexOf("no-syntax-error.css")).not.toBe(-1);
          |                                                                  ^
      310 |   });
      311 | 
      312 |   it("no-syntax-error.css linted", () => {
      at Object.toBe (lib/__tests__/ignore.test.js:309:66)

Anyone got an idea why?

@evilebottnawi

This comment has been minimized.

Show comment
Hide comment
@evilebottnawi

evilebottnawi Oct 15, 2018

Member

@jeddy3 maybe -1 is string?

Member

evilebottnawi commented Oct 15, 2018

@jeddy3 maybe -1 is string?

@jeddy3 jeddy3 merged commit 9878260 into master Oct 16, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.09%) to 96.281%
Details

@jeddy3 jeddy3 deleted the postcss-less-3 branch Oct 16, 2018

@jeddy3

This comment has been minimized.

Show comment
Hide comment
@jeddy3

jeddy3 Oct 16, 2018

Member

@jwilsson Thanks for this! Sorry about the delay in getting it merged.

Member

jeddy3 commented Oct 16, 2018

@jwilsson Thanks for this! Sorry about the delay in getting it merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment