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

Update Stylelint in peer dependency #22

Merged
merged 2 commits into from
Feb 19, 2024
Merged

Update Stylelint in peer dependency #22

merged 2 commits into from
Feb 19, 2024

Conversation

ai
Copy link
Sponsor Contributor

@ai ai commented Dec 8, 2023

@ai
Copy link
Sponsor Contributor Author

ai commented Dec 8, 2023

Hm. Right now we have a warning:

CommonJS plugins are deprecated ("/home/ai/Dev/slowreader/node_modules/.pnpm/stylelint-use-logical@2.1.0_stylelint@16.0.1/node_modules/stylelint-use-logical/index.cjs"). See https://stylelint.io/migration-guide/to-16

And error:

Unknown rule csstools/use-logical  csstools/use-logical

@ai
Copy link
Sponsor Contributor Author

ai commented Dec 8, 2023

Seems like it happens because this repo is using Rollup to compile ESM to CJS because stylelint-gamut (which is also written on pure CJS) has no problems.

@ybiquitous
Copy link

Hi, I'm a maintainer of Stylelint. The Unknown rule csstools/use-logical error is caused by the incorrect exporting ruleName:

export { ruleName }

Although this code works with Stylelint v15, it's an undocumented behavior. See the v15 plugin documentation.

I strongly recommend fixing this plugin as follows. It works with both v15 and v16:

--- a/index.js
+++ b/index.js
@@ -7,7 +7,7 @@ import walk from './lib/walk';
 
 const reportedDecls = new WeakMap();
 
-export default stylelint.createPlugin(ruleName, (method, opts, context) => {
+function ruleFunc(method, opts, context) {
 	const propExceptions = [].concat(Object(opts).except || []);
 	const isAutofix = isContextAutofixing(context);
 	const dir = /^rtl$/i.test(Object(opts).direction) ? 'rtl' : 'ltr';
@@ -140,9 +140,11 @@ export default stylelint.createPlugin(ruleName, (method, opts, context) => {
 			});
 		}
 	};
-});
+}
 
-export { ruleName }
+ruleFunc.ruleName = ruleName;
+
+export default stylelint.createPlugin(ruleName, ruleFunc);
 
 const isMethodIndifferent = method => method === 'ignore' || method === false || method === null;
 const isMethodAlways = method => method === 'always' || method === true;

@JounQin
Copy link

JounQin commented Dec 10, 2023

Is that a good practice to add properties onto function?

@ybiquitous
Copy link

Is that a good practice to add properties onto function?

I'm unsure if that's a good practice or not. But it respects compatibility.

@ai
Copy link
Sponsor Contributor Author

ai commented Dec 10, 2023

@JounQin sure. JS allows it and I do not know big disadvantages.

@JounQin
Copy link

JounQin commented Dec 10, 2023

I think we should add back the previous support for compatibility, and export default + ruleName should be preferred IMO.

@JounQin
Copy link

JounQin commented Dec 10, 2023

@JounQin sure. JS allows it and I do not know big disadvantages.

It doesn't work well with TypeScript by default.

JounQin added a commit to JounQin/stylelint-use-logical that referenced this pull request Dec 10, 2023
@ai
Copy link
Sponsor Contributor Author

ai commented Dec 10, 2023

I think we should add back the previous support for compatibility, and export default + ruleName should be preferred IMO.

We should move the discussion to Stylelint repo. Anyway this is a Stylelint current API.

@ai
Copy link
Sponsor Contributor Author

ai commented Dec 10, 2023

@jonathantneal I updated PR with @ybiquitous fixes

@@ -42,7 +42,7 @@
"stylelint-tape": "3.0.0"
},
"peerDependencies": {
"stylelint": ">= 11 < 16"
"stylelint": ">= 11 < 17"
Copy link

Choose a reason for hiding this comment

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

The new plugin structure works on >=11 <15? @ybiquitous

Choose a reason for hiding this comment

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

It's not a new structure.

Copy link

Choose a reason for hiding this comment

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

OK, so it's still compatible here, right?

Choose a reason for hiding this comment

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

Yes. export { ruleName } happens to work.

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

@ybiquitous Any thoughts?

Copy link
Sponsor Contributor Author

@ai ai Dec 10, 2023

Choose a reason for hiding this comment

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

The Stylelint API was not changed, we should not change this range.

We just moved to a correct API which worked before by accident and stop working in Stylelint 16.

Copy link

Choose a reason for hiding this comment

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

The Stylelint API was not changed, we should not change this range.

I'm just confirming whether current pattern also works on >=11 <15.

We just moved to a correct API which worked before by accident and stop working in Stylelint 16.

That's why I said for compatibility, stylelint should not remove the API, and the API is better than current one IMO.

@inomdzhon
Copy link

Hi, there!

Any updates?

@romainmenke
Copy link
Member

No updates yet, but I haven't forgotten.
Requires time to sort out publishing access :)

@inomdzhon
Copy link

No updates yet, but I haven't forgotten. Requires time to sort out publishing access :)

Hi, again)

Do you have any news about access?

@romainmenke
Copy link
Member

Thank you @ai, and everyone else 🙇

@romainmenke romainmenke merged commit 88d3fbd into csstools:main Feb 19, 2024
@romainmenke
Copy link
Member

This has been released as 2.1.1

@ramil-k
Copy link

ramil-k commented Feb 20, 2024

Thanks a lot for your work.

I've just migrated to 2.1.1 and I still getting an error message:
CommonJS plugins are deprecated ("/.../Frontend/node_modules/stylelint-use-logical/index.cjs"). See https://stylelint.io/migration-guide/to-16

Is there some configurations that I should update?

@romainmenke
Copy link
Member

romainmenke commented Feb 20, 2024

That message is kinda expected.
Stylelint is in the process of migrating to pure esm
In a future update we will update this plugin to pure esm.

But at this time it is only a warning and shouldn't be to much of a nuisance :)

Unless I misunderstood and that message is also a hard error?

@ramil-k
Copy link

ramil-k commented Feb 20, 2024

Oh, I see, thanks for clarification. It works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants