AndyOGo / stylelint-declaration-strict-value

Specify properties for which a variable, function, keyword or value must be used.
MIT License
134 stars 10 forks source link

add support to stylelint 16 bro #327

Closed ghost closed 10 months ago

ghost commented 11 months ago

Please support stylelint 16.0.0, the big one

Now the code is in ESM instead of CommonJS

Very good

Stylelint team published docs to know how to migrate with ease.

No need to keep support to commons from now on.

Style lint 16 is not allowed in your package.json. remove this restriction to

cslettengren commented 11 months ago

This doesn't seem to be completed? package.json still has "stylelint": "^15.1.0"

Zeeex commented 11 months ago

This plugin is definitely broken in stylelint v16, it doesn't recognize rules, please reopen.

AndyOGo commented 11 months ago

@cslettengren @Zeeex You are right.

Sorry, this issue has no issuer - just a @ghost

vitallium commented 11 months ago

@AndyOGo Hi, firstly, thanks for your stylelint plugin! Great job here, we use it in our stylelint config at GitLab. We are slowly migrating our configuration to the stylelint@16 and this plugin is the last dependency we need to update. I would like to help with that migration. I briefly looked at the repo and I think there is no straightforward migration, so before doing anything I want to share my plan with you to confirm that it looks good.

  1. One of the dependencies - https://github.com/stylelint/jest-preset-stylelint pinned the minimum required Node.js version to v18. Do you think this plugin should do the same?
  2. Additionally, the mentioned dep jest-preset-stylelint requires an upgrade to v7 to support stylelint@16.
  3. I tried to build the repo and got the following error:
npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: stylelint-declaration-strict-value@1.9.2
npm ERR! Found: typedoc@0.25.4
npm ERR! node_modules/typedoc
npm ERR!   dev typedoc@"^0.22.7" from the root project
npm ERR!   peer typedoc@">=0.24.0" from typedoc-plugin-markdown@3.17.1
npm ERR!   node_modules/typedoc-plugin-markdown
npm ERR!     dev typedoc-plugin-markdown@"^3.2.1" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! dev typedoc@"^0.22.7" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: typescript@4.7.4
npm ERR! node_modules/typescript
npm ERR!   peer typescript@"4.0.x || 4.1.x || 4.2.x || 4.3.x || 4.4.x || 4.5.x || 4.6.x || 4.7.x" from typedoc@0.22.18
npm ERR!   node_modules/typedoc
npm ERR!     dev typedoc@"^0.22.7" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

Seems like we need to update the typedoc dependency. Is that okay?

  1. After updating the typedoc dependency I got another error:
❯ npm run build

> stylelint-declaration-strict-value@1.9.2 build
> microbundle --tsconfig tsconfig.build.json --compress

(rpt2 plugin) TypeError: Cannot read properties of undefined (reading 'length')
TypeError: Cannot read properties of undefined (reading 'length')
    at getPropertyOfType (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:44635:45)
    at getExternalModuleMember (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:37183:46)
    at getTargetOfImportSpecifier (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:37267:28)
    at getTargetOfAliasDeclaration (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:37325:28)
    at resolveAlias (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:37364:30)
    at tryResolveAlias (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:37380:24)
    at getCandidateName (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:57441:33)
    at Object.getSpellingSuggestion (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:1883:33)
    at getSpellingSuggestionForName (/Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:57431:23)
    at /Users/vslobodin/Development/stylelint-declaration-strict-value/node_modules/microbundle/node_modules/typescript/lib/typescript.js:57376:34

Not sure what this error is about.

These just are some things that probably could be addressed, or probably not. Any way, before doing anything, I would like to get your thoughts. Thanks!

silverwind commented 11 months ago

I'm seeing a different error when I try to run this plugin on stylelint 16. Seems stylelint can not find the plugin anymore:

test.css
 1:1  ✖  Unknown rule scale-unlimited/declaration-strict-value  scale-unlimited/declaration-strict-value
AndyOGo commented 11 months ago

Stylelint always offers decent migration guides - most of the required steps are documented there.

https://stylelint.io/migration-guide/to-16

mattbrictson commented 10 months ago

My projects use this plugin and so I thought I would try my hand at getting it working with Stylelint 16. I followed the migration guide, which worked as far as making the plugin compatible Stylelint 16, but it broke compatibility with Stylelint 15 and older. In short, I couldn't figure out how to make a hybrid ESM/CJS plugin that works for both v15 and v16.

I'm hoping a person with more expertise in Node builds and packaging can solve this. If not, it would seem that the path forward would be to make this plugin ESM-only and drop support for Stylelint < 16. Is that the right move? I'm looking around at other Stylelint plugins and the stylelint-prettier plugin, for example, has chosen to go the ESM-only route and no longer works with v15: https://github.com/prettier/stylelint-prettier/blob/main/CHANGELOG.md#500-2023-12-10

Zeeex commented 10 months ago

I'm all for using ESM if it's the most doable solution. Most Stylelint plugins went that route, and is the way forward.

People can always use older version if the need arises, albeit with no new features, which would probably be ok.

AndyOGo commented 10 months ago

@cslettengren @mattbrictson @silverwind @vitallium @Zeeex @inomdzhon @amk221 Thanks for the issue.

I aimed for a non-breaking change - despite the fact that styelint v16 makes use of experimental ESM node features (IMHO I'm not fond of this decision - details are in the PR).

Released in v1.10.0 v1.10.3.

I hope that works well for you, if not please re-open.

A part from that I wish you a happy new year.

mattbrictson commented 10 months ago

Thanks! I just tested my project with 1.10.0 and I am still getting this error:

Unknown rule scale-unlimited/declaration-strict-value

I'm using:

AndyOGo commented 10 months ago

@mattbrictson Thx for your quick reply.

I had that same error in my tests and needed to migrate my jest config too. Diff in my PR: https://github.com/AndyOGo/stylelint-declaration-strict-value/pull/328/files#diff-4c3cfde2c8298e64ca709feb98ce653c1f25d523dc113f5fc097d9314ec785efR4-R8

I found that part of the migration in the docs here https://www.npmjs.com/package/jest-preset-stylelint#adjust-setup-globally

Does this help you?

AndyOGo commented 10 months ago

@mattbrictson I guess I had a typo... https://github.com/AndyOGo/stylelint-declaration-strict-value/pull/329/files

Released a fix in [v1.10.0](https://github.com/AndyOGo/stylelint-declaration-strict-value/blob/master/CHANGELOG.md#1101-2024-01-05.

I keep this issue open until this is confirmed.

Zeeex commented 10 months ago

stylelint-declaration-strict-value@1.10.1 still broken sadly 😢

Unknown rules still popups on v16, but on v15 works good. Using Node 20.

mattbrictson commented 10 months ago

I'm still running into the "unknown rule" error with 1.10.1.

Here is a minimal repro: https://github.com/mattbrictson/stylelint-repro

mattbrictson commented 10 months ago

No idea if this is the right solution, but it works for me. I had to completely remove the CJS export. After that Node complained "Cannot find module shortcss/lib/list", which I had to fix by using shortcss/lib/list.js.

Here's my diff:

diff --git package.json package.json
index b5d69c0..5d94fce 100644
--- package.json
+++ package.json
@@ -5,7 +5,6 @@
   "source": "src/index.ts",
   "exports": {
     "types": "./dist/index.d.ts",
-    "require": "./dist/index.js",
     "default": "./dist/index.modern.mjs"
   },
   "main": "dist/index.js",
diff --git src/index.ts src/index.ts
index fe069cc..15639b6 100644
--- src/index.ts
+++ src/index.ts
@@ -1,7 +1,7 @@
 import type { Declaration, Root, AtRule } from 'postcss';
 import stylelint, { PostcssResult, Rule } from 'stylelint';
 import shortCSS from 'shortcss';
-import list from 'shortcss/lib/list';
+import list from 'shortcss/lib/list.js';
 import cssValues from 'css-values';

 import {
diff --git types/shortcss.d.ts types/shortcss.d.ts
index 4df04d6..7043e0a 100644
--- types/shortcss.d.ts
+++ types/shortcss.d.ts
@@ -1,3 +1,3 @@
 declare module 'shortcss';

-declare module 'shortcss/lib/list';
+declare module 'shortcss/lib/list.js';

I can put this up as a PR if you want.

AndyOGo commented 10 months ago

@mattbrictson I got this config working by moving it to .mjs:

.stylelintrc.mjs

import x from "stylelint-declaration-strict-value"

export default {
  plugins: [x],
  rules: {
    "scale-unlimited/declaration-strict-value": [
      "/color/",
      {
        disableFix: true,
        ignoreValues: [
          "currentcolor",
          "inherit",
          "initial",
          "transparent",
          "unset",
          "white",
        ],
      },
    ],
  },
};

But your example .stylelintrc.js still fails 🤔

mattbrictson commented 10 months ago

@AndyOGo good enough for me! I am no longer blocked in using Stylelint 16. I can change my stylelintrc file to ESM and then explicitly import the plugin as you show in your example. I just confirmed this works with 1.10.2.

AndyOGo commented 10 months ago

@Zeeex @mattbrictson Thank you for helping out.

I got your reproduction .stylelintrc.js working :)

Released in v1.10.3.

Demo: https://stylelint.io/demo/#N4Igxg9gJgpiBcICGACYAdAdugLpANhAE7woDEAjFQNxYC+WIANCAGYCW+MAckgLZxEMAB78ADlwB0YAM4zm4CJg4BzBCAyYUKdCBE4YmKDN2kA2lm3bdMnAE8u+dphwBaSMvYrXtpEaREULqWKAC6TCG6EgCuKs4mCCgWWlY29o7ObrBg+AFIOOxKPjhE7GBuAG5I+NEwwSnhkSBE0VwJpJpWOiAyYNUwrtGYTnzsBlAA9Nm5RPmFmMWl5a5VNXWJugTE9doMmHQKHqoAYsR8+eoAVjJKCrBi8oid3bYOME4upt25Bra6ESk0m8Pm4jl5in4oAEghsQD8YH9mE1XhkXK5pnkCkVbEtKtVal9dPDEfQQHQgA

mattbrictson commented 10 months ago

Confirmed that 1.10.3 works with my original .stylelintrc.js. Thanks! 🙏