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

Change extends in overrides to be consistent with plugins behaviour #6380

Merged
merged 14 commits into from Oct 27, 2022

Conversation

jasikpark
Copy link
Contributor

@jasikpark jasikpark commented Sep 30, 2022

  • Adds docs describing how different properties get merged in overrides
  • Merges extends arrays together in overrides
  • Adds a test for that extends merging functionality

@changeset-bot
Copy link

changeset-bot bot commented Sep 30, 2022

🦋 Changeset detected

Latest commit: 01cd74e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jasikpark
Copy link
Contributor Author

test output:

 FAIL  lib/__tests__/applyOverrides.test.js
  /** snip */

  ● two matching overrides › with extends

    expect(received).toEqual(expected) // deep equality

    - Expected  - 2
    + Received  + 0

    @@ -1,9 +1,7 @@
      Object {
        "extends": Array [
    -     "stylelint-config1",
    -     "stylelint-config2",
          "stylelint-config3",
        ],
        "rules": Object {
          "block-no-empty": null,
          "color-no-hex": true,

      236 |             const applied = applyOverrides(config, __dirname, path.join(__dirname, 'style.module.css'));
      237 |
    > 238 |             expect(applied).toEqual(expectedConfig);
          |                             ^
      239 |     });
      240 | });
      241 |

      at Object.toEqual (lib/__tests__/applyOverrides.test.js:238:19)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 12 passed, 13 total
Snapshots:   2 passed, 2 total
Time:        0.331 s, estimated 1 s
Ran all test suites related to changed files.

Watch Usage: Press w to show more.

@jeddy3
Copy link
Member

jeddy3 commented Oct 2, 2022

@jasikpark Thanks for opening this pull request to show the current behaviour.

If this is the intended behavior

I've been looking back through some issues (#5603, #5656 etc) and it doesn't look like we ever discussed what the intended behaviour should be.

It feels safe to merge plugins as it simply increases the pool of available rules. extends, on the other hand, can change what rules are turned on and off. However, having consistent behaviour for both may make the most sense.

Although the current behaviour is undocumented, we can't know how many users rely on it.

Anyone else got thoughts on this?

Whatever we decide, let's update our docs to describe the behaviour for each of the most used configuration properties: rules, customSyntax, extends and plugins.

At the moment, I believe it's:

  • rules - merge
  • customSyntax - replace
  • extends - replace
  • plugins - merge

@jeddy3 jeddy3 changed the title Add test for passing extends to stylelint overrides Add test for using extends in overrides Oct 2, 2022
@ybiquitous
Copy link
Member

@jeddy3 Thanks for summarizing the issues.

extends, on the other hand, can change what rules are turned on and off. However, having consistent behaviour for both may make the most sense.

I agree with the idea that extends should cover merging like plugins.
And, also updating our docs. 👍🏼

@jeddy3
Copy link
Member

jeddy3 commented Oct 3, 2022

OK, let's make the extends behaviour consistent with plugins. It was an oversight, so shall we classify as a bug fix?

@jasikpark Feel free to fix the behaviour in this pull request and update the docs.

For the docs, we can add a bullet at the end of this section.

  • Configuration properties with a single value, e.g. customSyntax, will be overridden whereas everything else, e.g. extends, plugins, rules etc., will be merged.

@jasikpark jasikpark force-pushed the test-stylelint-override-extends branch from 7ea9f8a to d471842 Compare October 4, 2022 21:17
@jasikpark
Copy link
Contributor Author

Okay, pushed some basics docs & merging extends. What is the explicit list of things that should be merged vs replaced?

@jasikpark
Copy link
Contributor Author

looks like rules are not deduplicated, though they are merged

https://github.com/jasikpark/stylelint/blob/7ac953e87cef7da2b7264220fd766b361e4ee5b5/lib/augmentConfig.js#L236-L246

@jasikpark jasikpark changed the title Add test for using extends in overrides Merge extends and other complex config properties in overrides Oct 4, 2022
@jasikpark
Copy link
Contributor Author

I guess everything in

export type Config = {
extends?: ConfigExtends;
plugins?: ConfigPlugins;
pluginFunctions?: {
[pluginName: string]: Rule;
};
processors?: ConfigProcessors;
processorFunctions?: Function[];
ignoreFiles?: ConfigIgnoreFiles;
ignorePatterns?: string;
rules?: ConfigRules;
codeProcessors?: CodeProcessor[];
resultProcessors?: ResultProcessor[];
quiet?: boolean;
defaultSeverity?: Severity;
ignoreDisables?: DisableSettings;
reportNeedlessDisables?: DisableSettings;
reportInvalidScopeDisables?: DisableSettings;
reportDescriptionlessDisables?: DisableSettings;
overrides?: ConfigOverride[];
customSyntax?: CustomSyntax;
};

that is an array should be merged right? since technically any of those can be passed to overrides?

@jasikpark
Copy link
Contributor Author

might be worth looking to eslint's behavior here as well https://eslint.org/docs/latest/user-guide/configuring/configuration-files#how-do-overrides-work

@jasikpark
Copy link
Contributor Author

Seems that extends, plugins, processors, processorFunctions, ignoreFiles, codeProcessors, resultProcessors are arrays and might be merged similarly to the existing plugins behavior

rules and pluginFunctions are objects containing a variable number of properties, which could also be merged potentially

ignorePatterns, quiet, defaultSeverity, ignoreDisables, report*, and customSyntax are javscript built-in datatypes like boolean and string and should be replaced.

I'd suggest maybe this PR can just alter extends behavior and then the other config values can be updated in a second PR?

@jasikpark
Copy link
Contributor Author

Since I'm not sure what the semantics are of all these config values

@jeddy3 jeddy3 changed the title Merge extends and other complex config properties in overrides Fix extends in overrides to be consistent with plugins behaviour Oct 6, 2022
@jeddy3
Copy link
Member

jeddy3 commented Oct 6, 2022

@jasikpark Thanks for making the changes and for digging more into the overrideable options.

I'd suggest maybe this PR can just alter extends behavior and then the other config values can be updated in a second PR?

That sounds best. The general behaviour you outline in #6380 (comment), SGTM.

We're going to remove Processors in the next major release. That leaves the following as user-facing options:

  • extends
  • plugins
  • ignoreFiles
  • quiet
  • defaultSeverity
  • ignoreDisables
  • report*
  • customSyntax

If we haven't already, it'd be good to have tests for these.

I've added a changeset entry. I think this pull request is ready for review.

@jasikpark
Copy link
Contributor Author

I suppose my documentation update is inaccurate now if I'm only going to alter extends 😓

Maybe I should add tests + docs that describe where we are today && then another PR can fix everything?

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.

Maybe I should add tests + docs that describe where we are today && then another PR can fix everything?

Would you mind doing that in another pull request?

I've made a suggestion to tweak the docs for this pull request.

docs/user-guide/configure.md Outdated Show resolved Hide resolved
@jeddy3 jeddy3 changed the title Fix extends in overrides to be consistent with plugins behaviour Change extends in overrides to be consistent with plugins behaviour Oct 11, 2022
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.

Thanks, LGTM 👍🏼

@jasikpark jasikpark force-pushed the test-stylelint-override-extends branch from ed62a9c to 1e176e4 Compare October 12, 2022 22:34
@jasikpark jasikpark requested a review from jeddy3 October 13, 2022 15:28
@jeddy3 jeddy3 mentioned this pull request Oct 13, 2022
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.

LGTM, thank you!

We can merge once we start actively working on a v15 branch.

I've created a tracking issue for preparing 15.0.0 (#6409).

@jeddy3 jeddy3 added the pr: blocked is blocked by another issue or pr label Oct 13, 2022
@ybiquitous ybiquitous changed the base branch from main to v15 October 24, 2022 02:26
@ybiquitous
Copy link
Member

I've updated the PR base branch to v15. Can we merge this PR? @jeddy3

@jeddy3 jeddy3 removed the pr: blocked is blocked by another issue or pr label Oct 24, 2022
@jeddy3
Copy link
Member

jeddy3 commented Oct 24, 2022

I've moved the migrating information out of the changeset and into a migration guide doc.

I've updated the PR base branch to v15. Can we merge this PR? @jeddy3

Yes.

@jeddy3
Copy link
Member

jeddy3 commented Oct 24, 2022

This merge behaviour is aligned with how a flat config would work, and so is a step in the right direction for us.

From this article:

Inside of the array, ESLint finds all config objects that match the file being linted and merges them together in much the same way that eslintrc did. The only real difference is the merge happens from the top of the array down to the bottom instead of using files in a directory structure.

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.

@jeddy3 Thanks so much for writing up the migration guide. There may be a few typos left, but LGTM! 👍🏼

docs/migration-guide/to-15.md Outdated Show resolved Hide resolved
docs/migration-guide/to-15.md Outdated Show resolved Hide resolved
@jasikpark
Copy link
Contributor Author

thx for getting this merge ready 💜

jeddy3 and others added 2 commits October 25, 2022 10:14
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@ybiquitous
Copy link
Member

We can merge this when CI passes. 💪🏼

@ybiquitous ybiquitous merged commit 4daef24 into stylelint:v15 Oct 27, 2022
ybiquitous added a commit that referenced this pull request Nov 8, 2022
…iour (#6380)

Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
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