Skip to content

Conversation

baseballyama
Copy link
Member

close part of #1353
close part of #1327

Copy link

changeset-bot bot commented Sep 24, 2025

🦋 Changeset detected

Latest commit: db65d65

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

This PR includes changesets to release 1 package
Name Type
eslint-plugin-svelte Patch

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

Copy link
Contributor

github-actions bot commented Sep 24, 2025

Try the Instant Preview in Online Playground

ESLint Online Playground

Install the Instant Preview to Your Local

npm i https://pkg.pr.new/eslint-plugin-svelte@db65d65

Published Instant Preview Packages:

View Commit

@baseballyama baseballyama force-pushed the fix/no-navigation-without-resolve-search-param branch from 425224c to db65d65 Compare September 24, 2025 02:12
@marekdedic
Copy link
Contributor

I think this should be the default behaviour. I'll take a look at the code in a few days - I have some other ideas for improvements as well, so would like to push it in a certain way...

@marekdedic
Copy link
Contributor

marekdedic commented Oct 5, 2025

Hi,
thanks for the PR, it generally looks like the right idea. Several points about the design

  1. I think this doesn't need an option gating it, suffixes should always be allowed
  2. I think this should be checked for both resolve() as well as asset() - while not common, even assets can have query options.

Those 2 should simplify the code significantly. On the other hand, one more complication:

  1. I don't think we should just allow any suffix. My first idea would be to check that the suffix starts with either a ? or a # - I think that would be a reasonable trade-off between complexity and correctness

Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be in this PR? I am confused.

return isResolveCall(ctx, variable.identifiers[0].parent.init, resolveReferences);
}

function isResolveWithOptionalSuffix(
Copy link
Contributor

Choose a reason for hiding this comment

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

2 notes on the design:

  1. Can we split this function so that it has just 1 responsibility? Now it's checking for resolve and the suffix, IMO it would be better to have 2 functions (and we already have one of them...)
  2. I'd prefer to split this function by expression type, it feels easier to read the code that way

})
);

function onlyCallExpressions(list: TrackedReferences<boolean>[]): TSESTree.CallExpression[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why this is needed? I don't get it :/

);
}

function expressionIsAbsolute(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PR should remove/repurpose this function

);
}

function urlValueIsAbsolute(url: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PR should remove/repurpose this function

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.

2 participants