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

Remove support for Stylelint less than 16.0.0 #94

Merged
merged 1 commit into from
Dec 8, 2023
Merged

Remove support for Stylelint less than 16.0.0 #94

merged 1 commit into from
Dec 8, 2023

Conversation

jeddy3
Copy link
Owner

@jeddy3 jeddy3 commented Nov 15, 2023

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

Closes #95

Is there anything in the PR that needs further explanation?

Support for stylelint@16 onwards.

@jeddy3 jeddy3 mentioned this pull request Nov 15, 2023
10 tasks
package.json Outdated Show resolved Hide resolved
@ybiquitous
Copy link

@jeddy3 I found a workaround for the segfault error. Please try using jest-light-runner as suggested on nodejs/node#35889 (comment), although this runner lacks some features like coverage.

However, I don't identify why segfault errors don't occur in the stylelint test suite, but in this test suite. 😓

Related: stylelint/stylelint#7329

@jeddy3
Copy link
Owner Author

jeddy3 commented Nov 17, 2023

@ybiquitous Thanks for digging into some more! I've pushed a commit to use the light runner. Unfortunately, Windows is now tripping up with this error:

 Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:'

Prettier is using the runner on their codebase, so we can be confident it should work on Windows. It could be something with our preset or with how I'm configuring Jest (although, I've tried to match Prettier's config settings). I try to look into it myself over the weekend, but please share if you have any ideas.

@ybiquitous
Copy link

Um, I'm not 100% sure, but how about trying the loadLint option of jest-preset-stylelint?

const loadLint = () => import("stylelint").then((m) => m.default.lint);

global.testRule = getTestRule({ plugins: ["./lib/index.js"], loadLint });

See also:

@jeddy3
Copy link
Owner Author

jeddy3 commented Nov 17, 2023

@ybiquitous Thank you for the suggestion. I tried, but the error remained.

Debugging on Windows is a pain without a Windows machine to hand... pushing commits and waiting on the CI. However, I continued digging.

I remembered we could pass plugin objects directly to the plugins: [] configuration property. I've worked around the issue by dynamically importing the plugin in the jest setup:

import { getTestRule } from "jest-preset-stylelint";
const plugin = await import("./lib/index.js");
global.testRule = getTestRule({ plugins: [plugin] });

Is this an acceptable workaround to document in the migration guide, or should we dig deeper to discover why plugin locators aren't resolving correctly in Jest on Windows? I did try using fileURLToPath but then neither Windows nor Unix could find the file.

@ybiquitous
Copy link

ybiquitous commented Nov 17, 2023

Congrats for the CI green! 🎉

Is this an acceptable workaround to document in the migration guide, or should we dig deeper to discover why plugin locators aren't resolving correctly in Jest on Windows? I did try using fileURLToPath but then neither Windows nor Unix could find the file.

If we will create a new helper using node:test and recommend it instead of jest-preset-stylelint, it may be better to add the way to use global.testRule to the documents. 🤔

I guess it's more straightforward to import a plugin in its test (like fb5cd86) than a global setup file (this feature is specific for Jest at this time). For example:

// test file per rule
import rule from "../index.js";

const {
  rule: { messages, ruleName },
} = rule;

testRule({
  // 🙆🏼‍♂️ Good
  plugins: [rule],

  // 🙅🏼‍♂️ Bad
  plugins: ["./index.js"],

  ruleName,
  // other settings...
});

//-------------------//

// 🙅🏼‍♂️ Bad
// jest.setup.js
global.testRule = getTestRule({
  plugins: [plugin],
});

If this would makes sense, it seems we should update the plugin testing guide and the jest-preset-stylelint README, in addition to the migration guide for v16.

Just to confirm, the way to pass a plugin object directly would never raise a problem even in Windows, right?

@jeddy3
Copy link
Owner Author

jeddy3 commented Nov 17, 2023

I guess it's more straightforward to import a plugin in its test (like fb5cd86) than a global setup file (this feature is specific for Jest at this time)

Yes, I agree. It feels like passing the plugin object directly is:

  • cleaner, rather than a pass locator that Stylelint resolves
  • a pattern that'll work across all runners, like a new node:test one

It seems we should update the plugin testing guide and the jest-preset-stylelint README, in addition to the migration guide for v16.

I agree.

We don't need to dig deeper into the issue as we'll push this pattern in our docs because it's future-facing.

Just to confirm, the way to pass a plugin object directly would never raise a problem even in Windows, right?

I don't believe so. Stylelint has always accepted a plugin object directly in the plugins: [] configuration property.

@jeddy3 jeddy3 changed the title Change to ESM Remove support for Stylelint less than 16.0.0 Dec 8, 2023
@jeddy3 jeddy3 merged commit a90f76e into main Dec 8, 2023
5 checks passed
@jeddy3 jeddy3 deleted the esm branch December 8, 2023 14:19
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.

Support Stylelint@16
2 participants