feat(no-sync)!: move `ts-declaration-location` to `peerDependencies` #451

risantos · 2025-06-26T09:00:24Z

Description

Move ts-declaration-location from dependencies to devDependencies and peerDependencies, declaring it as optional on peerDependenciesMeta, which has typescript as a peer dependency.
As ts-declaration-location is already tentatively loaded for n/no-sync, the plugin keeps working as expected, throwing with a message identifying the missing package in case it'd be needed while trying to use the ignores option with a non-string value.
Update the n/no-sync documentation to refer the updated peer dependency requirement.
Update n/no-sync rule to conditionally require ts-declaration-location only when advanced ignores are used.
Add typescript as an optional peer dependency for the n/no-sync rule, used on lib/util/get-full-type-name.js, removing unused eslint-disable directive n/no-unpublished-require.

Note

~~Set as feat as this should be at least a minor bump, as it may break a lint build.~~
Updated to feat! after discussion, as it's a breaking change for TypeScript users using advanced TypeScript-based ignores.

Related issues

Warning received while installing eslint-plugin-n on JS projects without typescript:

warning "eslint-plugin-n > @typescript-eslint/[email protected]" has unmet peer dependency "typescript@>=4.8.4 <5.9.0".
warning "eslint-plugin-n > [email protected]" has unmet peer dependency "typescript@>=4.0.0".
warning "eslint-plugin-n > @typescript-eslint/utils > @typescript-eslint/[email protected]" has unmet peer dependency "typescript@>=4.8.4 <5.9.0".
warning "eslint-plugin-n > @typescript-eslint/utils > @typescript-eslint/typescript-estree > @typescript-eslint/[email protected]" has unmet peer dependency "typescript@>=4.8.4 <5.9.0".
warning "eslint-plugin-n > @typescript-eslint/utils > @typescript-eslint/typescript-estree > @typescript-eslint/[email protected]" has unmet peer dependency "typescript@>=4.8.4 <5.9.0".
warning "eslint-plugin-n > @typescript-eslint/utils > @typescript-eslint/typescript-estree > [email protected]" has unmet peer dependency "typescript@>=4.8.4".

chore(no-sync): remove @typescript-eslint/utils #449 addressed @typescript-eslint/utils.

aladdin-add

is it possible to move the require(), so it is loaded only when the option is configured?

MichaelDeBoey

This would be a breaking change for people using the ignores option of the no-sync rule

aladdin-add · 2025-06-26T12:22:47Z

the option was introduced in a minor (https://github.com/eslint-community/eslint-plugin-n/releases/tag/v17.19.0), but it unexpectedly introduced ts requirement.

so technically it can be a bugfix?

risantos · 2025-06-26T14:16:10Z

@aladdin-add

is it possible to move the require(), so it is loaded only when the option is configured?

Yeah, moved below with a check for the existence of non-string option.ignores.

@MichaelDeBoey

This would be a breaking change for people using the ignores option of the no-sync rule

Likely so, I had read it as minor, given it "might break your lint build", being specific to TS users using n/no-sync with advanced ignores.

However, as I have no data/visibility over how this is used, "likely to break your lint build" also applies.

@aladdin-add

so technically it can be a bugfix?

I could see it working as such having been introduced 24d ago, although if someone's using it, it'd throw. Solvable by adding ts-declaration-location, but still "breaking".

scagood · 2025-06-27T11:07:51Z

Could we added typescript as optional peer dependency to mitigate this instead of ts-declaration-location?

risantos · 2025-06-27T18:23:03Z

@scagood

Could we added typescript as optional peer dependency to mitigate this instead of ts-declaration-location?

I've tested adding typescript as an optional peer dependency instead, but the warning remains:

warning "eslint-plugin-n > [email protected]" has unmet peer dependency "typescript@>=4.0.0".

However, I believe it should be added too nevertheless, given typescript is required on lib/util/get-full-type-name.js

aladdin-add · 2025-06-27T19:58:26Z

After some thought, I was wondering if we could ask ts-declaration-location to make some changes -- change ts to optional. (I'm guessing that looking for the declared location isn't really needed to run tsc 😄 )

update: it was needed: https://github.com/RebeccaStevens/ts-declaration-location/blob/d86b36fc0dcba98fb964e58b8c2e7e7614fffd23/src/main.ts#L5 😂

aladdin-add

the change LGTM, thanks for taking care of it! 👍

aladdin-add · 2025-06-27T20:17:19Z

If we can go back to reintroduce this feature—I thought it was currently the only option (without breaking non-TypeScript projects).

Set as feat as this should be at least a minor bump, as it may break a lint build.

I agreed it to be a minor. thoughts? @scagood @MichaelDeBoey

scagood · 2025-06-27T21:21:42Z

mmm, this is a breaking change though, I would rather stick with semver for this if we do go ahead with it. 👀

@RebeccaStevens what do you think about this?

RebeccaStevens · 2025-06-27T21:46:16Z

I'm in favour of this change.

It is technically a breaking chance so I would recommend a version bump but that's up to you whether or not you want to do that.

docs/rules/no-sync.md

RebeccaStevens · 2025-06-27T21:52:10Z

lib/rules/no-sync.js

@@ -124,6 +112,29 @@ module.exports = {
        const selector = options.allowAtRootLevel
            ? selectors.map(selector => `:function ${selector}`)
            : selectors
+
+        // Only require `ts-declaration-location` if needed.
+        let typeMatchesSpecifier =


Let's keep the variable defined at the top level and add a check below to see if it's undefined or not before loading the library. That way the library isn't loaded multiple times.

Additionally we could add a check to not try and load it if we've already tried before.

The way I'd do this is have the variable initialized to undefined. When we encounter the code below, if the variable is set to undefined, test if we need to load the library, if we do, do so and set this variable to it, otherwise set the variable to null.

it may be a bit faster, even requires() are always cached. :)

Updated based on what I understood from your comments, please check whether the change is aligned. 🙏🏼

lib/rules/no-sync.js

risantos · 2025-06-30T11:53:54Z

@aladdin-add

If we can go back to reintroduce this feature—I thought it was currently the only option (without breaking non-TypeScript projects).

Set as feat as this should be at least a minor bump, as it may break a lint build.

I agreed it to be a minor. thoughts? @scagood @MichaelDeBoey

Updated the PR description to reflect that it's a breaking change, too.

aladdin-add · 2025-06-30T14:07:38Z

lib/rules/no-sync.js

+        if (hasAdvancedIgnores) {
+            if (typeMatchesSpecifier === undefined) {
+                try {
+                    typeMatchesSpecifier =
+                        /** @type {import('ts-declaration-location').default} */ (
+                            /** @type {unknown} */ (
+                                require("ts-declaration-location")
+                        )
+                    )
+                } catch {
+                    typeMatchesSpecifier === null
+                }
+            } else {
+                throw new Error(
+                    'ts-declaration-location not available. Rule "n/no-sync" is configured to use "ignores" option with a non-string value. This requires ts-declaration-location to be available.'
+                )
+            }
+        }
+


Isn't this the most intuitive way?

try{ typeMatchesSpecifier = typeMatchesSpecifier || require("ts-declaration-location"); } catch(e) { throw new Error( 'ts-declaration-location not available. Rule "n/no-sync" is configured to use "ignores" option with a non-string value. This requires ts-declaration-location to be available.', { cause: e } ); }

It is clearer, thank you.
Applied now with the Logical OR assignment operator.

scagood · 2025-06-30T23:15:02Z

The code is 👍 from me, so thank you!

The one thing I am missing here is a couple of tests. For example: when ts-declaration-location is not installed, when typescript is not installed, and when neither are installed.

I am not sure what the best way to pull this off though:
We could use something like Sinon or even mock.module when it becomes stable.
The other option I can think of is to make some "fixture" style tests that we run separately.

What do you think?

I don't mean to block this PR with this comment, I am happy for it to go as is, I would just prefer tests! 😅

risantos · 2025-07-02T16:48:22Z

@scagood

The one thing I am missing here is a couple of tests. For example: when ts-declaration-location is not installed, when typescript is not installed, and when neither are installed.

I am not sure what the best way to pull this off though: We could use something like Sinon or even mock.module when it becomes stable. The other option I can think of is to make some "fixture" style tests that we run separately.

What do you think?

Agreed on the need for tests. I'm used to jest or vitest for these, having mocking built-in mocking capability. I've already used the native MockModuleContext too, though it's still in an Experimental/development stage.

If using mocha, would it be ok to add sinon and proxyquire to test these?

scagood · 2025-07-03T23:01:37Z

If using mocha, would it be ok to add sinon and proxyquire to test these?

I think we'd only need sinon, right? If you need proxyquire do add it, and we can go from there :D

- Add `typescript` as an optional peer dependency for the `n/no-sync` rule, used on `lib/util/get-full-type-name.js`. - Set `ts-declaration-location` as an optional peer dependency for the `n/no-sync` rule. - Move `ts-declaration-location` to `devDependencies` for development and testing. - Update the `n/no-sync` documentation to refer the updated peer dependency requirement. - Update `n/no-sync` rule to conditionally require `ts-declaration-location` only when advanced ignores are used. - Remove unused eslint-disable directive n/no-unpublished-require on `lib/util/get-full-type-name.js`, as `typescript` is now an optional peer dependency. - Add tests to ensure the rule throws an error when `ts-declaration-location` or TypeScript parser services are not available. - Add `sinon` as a dev dependency, used for mocking in tests.

risantos · 2025-07-09T00:20:53Z

@scagood

If using mocha, would it be ok to add sinon and proxyquire to test these?

I think we'd only need sinon, right? If you need proxyquire do add it, and we can go from there :D

Thanks for confirming. I've added tests using sinon for the 3 scenarios you had described:

If missing ts-declaration-location.
If missing typescript.
If missing both.

WDYT?

scagood

This looks good to me 👍

Thanks

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch 2 times, most recently from 271865c to 02db95b Compare June 26, 2025 11:54

aladdin-add reviewed Jun 26, 2025

View reviewed changes

MichaelDeBoey added the breaking label Jun 26, 2025

MichaelDeBoey reviewed Jun 26, 2025

View reviewed changes

MichaelDeBoey changed the title ~~feat(no-sync): move ts-declaration-location to peerDependencies~~ feat(no-sync)!: move ts-declaration-location to peerDependencies Jun 26, 2025

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch from 02db95b to eeb4127 Compare June 26, 2025 14:28

ota-meshi mentioned this pull request Jun 26, 2025

chore(no-sync): remove @typescript-eslint/utils #449

Merged

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch from eeb4127 to 54166a2 Compare June 27, 2025 18:22

aladdin-add approved these changes Jun 27, 2025

View reviewed changes

RebeccaStevens suggested changes Jun 27, 2025

View reviewed changes

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch from 54166a2 to 1d93fdf Compare June 28, 2025 17:47

risantos requested a review from RebeccaStevens June 28, 2025 17:54

RebeccaStevens suggested changes Jun 28, 2025

View reviewed changes

lib/rules/no-sync.js Outdated Show resolved Hide resolved

RebeccaStevens reviewed Jun 28, 2025

View reviewed changes

lib/rules/no-sync.js Show resolved Hide resolved

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch from 1d93fdf to 9c75763 Compare June 30, 2025 11:50

risantos requested a review from RebeccaStevens June 30, 2025 11:50

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch from 9c75763 to 686880f Compare June 30, 2025 11:56

aladdin-add reviewed Jun 30, 2025

View reviewed changes

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch from 686880f to a108985 Compare June 30, 2025 15:27

RebeccaStevens approved these changes Jun 30, 2025

View reviewed changes

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch from a108985 to 627f17b Compare July 2, 2025 17:08

risantos force-pushed the feat/no-sync/optional-ts-declaration-location branch from 627f17b to 8e626ca Compare July 9, 2025 00:17

scagood approved these changes Jul 9, 2025

View reviewed changes

Uh oh!

feat(no-sync)!: move ts-declaration-location to peerDependencies #451

Are you sure you want to change the base?

feat(no-sync)!: move ts-declaration-location to peerDependencies #451

Conversation

risantos commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Uh oh!

aladdin-add left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelDeBoey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aladdin-add commented Jun 26, 2025

Uh oh!

risantos commented Jun 26, 2025

Uh oh!

scagood commented Jun 27, 2025

Uh oh!

risantos commented Jun 27, 2025

Uh oh!

aladdin-add commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aladdin-add left a comment

Choose a reason for hiding this comment

Uh oh!

aladdin-add commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scagood commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RebeccaStevens commented Jun 27, 2025

Uh oh!

Uh oh!

RebeccaStevens Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aladdin-add Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

risantos Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

risantos commented Jun 30, 2025

Uh oh!

aladdin-add Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

risantos Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

scagood commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

risantos commented Jul 2, 2025

Uh oh!

scagood commented Jul 3, 2025

Uh oh!

risantos commented Jul 9, 2025

Uh oh!

scagood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat(no-sync)!: move `ts-declaration-location` to `peerDependencies` #451

feat(no-sync)!: move `ts-declaration-location` to `peerDependencies` #451

risantos commented Jun 26, 2025 •

edited

Loading

MichaelDeBoey left a comment •

edited

Loading

aladdin-add commented Jun 27, 2025 •

edited

Loading

aladdin-add commented Jun 27, 2025 •

edited

Loading

scagood commented Jun 27, 2025 •

edited

Loading

RebeccaStevens Jun 27, 2025 •

edited

Loading

aladdin-add Jun 27, 2025 •

edited

Loading

aladdin-add Jun 30, 2025 •

edited

Loading

scagood commented Jun 30, 2025 •

edited

Loading