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

feat: Add svelte/valid-context-access rule #480

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

baseballyama
Copy link
Member

close: #448

@changeset-bot
Copy link

changeset-bot bot commented May 14, 2023

🦋 Changeset detected

Latest commit: ffc0bce

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 Minor

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

@baseballyama baseballyama force-pushed the feat/448 branch 2 times, most recently from 49ff328 to 43d6041 Compare May 14, 2023 11:08
@baseballyama baseballyama marked this pull request as ready for review May 28, 2023 06:26
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!
I haven't checked all yet, but I have some comments.

currentNode: TSESTree.CallExpression,
) {
let { parent } = currentNode
if (parent?.type !== "ExpressionStatement") {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect an if statement like the following will not work:

if (hasContext("answer")) {
}

Could you add this test case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a basic test for if statement. I think it works fine.
Is this same understanding with you?

a86eac8

Copy link
Member

Choose a reason for hiding this comment

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

Sorry. I should have explained more.
I would like to add a test for if statements are used at the top level.

<script>
  import { hasContext } from "svelte"

  if (hasContext("answer")) {
    console.log("The answer exist")
  }
</script>

Copy link
Member Author

@baseballyama baseballyama May 29, 2023

Choose a reason for hiding this comment

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

Oh OK! I will add a test and fix logic.

},
type: "problem",
},
create(context) {
Copy link
Member

Choose a reason for hiding this comment

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

It probably doesn't work well other then *.svelte files, so I think the following guard is needed.

Suggested change
create(context) {
create(context) {
if (!context.parserServices.isSvelte) {
return {}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to add more test but even JS file can use onMount so I think we need to check JS files also.

import { setContext, onMount } from "svelte"

const something = () => {
  setContext("answer", 42)
}

onMount(() => something())

But at the same time, I found this pattern.
It should be work but now ESLint error occurs.
So I need to handle this.

import { setContext } from "svelte"

const something = () => {
  setContext("answer", 42)
}

const wrapper = (fn) => {
  fn()
}

wrapper(() => something())

In addition, I forgot to handle async/await.😇

const something = async () => {
  await Promise.resolve()
  setContext("answer", 42)
}

something()

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this logic and I wrote this limitation on the docs.

@baseballyama
Copy link
Member Author

I realized that it’s impossible to check async/await things perfectly.
For example below case, it will be an error but I think it isn't easy to detect it by ESLint.

So my opinion is that we will mention this limitation on the docs and we don't check async/await things.
Or we don't implement this rule.
There may not be much value in the rule that includes this false negative.
What do you think @ota-meshi ?

<script>
import { setContext } from "svelte"
import someAsyncProcess from './outside'

someAsyncProcess(() => {
  setContext("answer", 42)
});
// outside.js
export async function someAsyncProcess(fn) {
  await Promise.resolve();
  fn();
}

@ota-meshi
Copy link
Member

ota-meshi commented May 30, 2023

Hmm. You're right, there are false negatives, but I don't think users will use setContext that way, so I don't think it matters much if it's not reported.

I think code like the following is a common mistake, so I think there's value in having it reported by a rule.

const something = async () => {
  await Promise.resolve()
  setContext("answer", 42)
}

eslint-plugin-vue has some similar checking rules.
https://eslint.vuejs.org/rules/no-expose-after-await.html
https://eslint.vuejs.org/rules/no-lifecycle-after-await.html
https://eslint.vuejs.org/rules/no-watch-after-await.html
https://eslint.vuejs.org/rules/no-restricted-call-after-await.html

Comment on lines +7 to +8
import { extractSvelteLifeCycleReferences } from "./reference-helpers/svelte-lifecycle"
import { extractTaskReferences } from "./reference-helpers/microtask"
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved few functions to utils dir because I want to use these in valid-context-access rule.

Comment on lines +357 to +361
const tickCallExpressions = Array.from(
extractSvelteLifeCycleReferences(context, ["tick"]),
)
const taskReferences = Array.from(extractTaskReferences(context))
const reactiveVariableReferences = getReactiveVariableReferences(context)
Copy link
Member Author

Choose a reason for hiding this comment

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

Just I moved there lines for performance improvement.

},
type: "problem",
},
create(context) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this logic and I wrote this limitation on the docs.

const awaitExpressions: {
belongingFunction:
| TSESTree.FunctionDeclaration
| TSESTree.VariableDeclaration
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the belongingFunction should be assigned a VariableDeclaration.
Should we have checked FunctionExpression instead?

function isAfterAwait(node: TSESTree.CallExpression) {
for (const awaitExpression of awaitExpressions) {
const { belongingFunction, node: awaitNode } = awaitExpression
if (isInsideOf(node, belongingFunction)) {
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be false positives in complex cases such as:

<script>
  import { getContext } from "svelte"

  async function fn() {
    async function foo() {
      await p
    }

    getContext("a")

    await foo()
  }
  fn()
</script>

I don't think we can check the correct function scope by just checking the range.

Comment on lines +165 to +197
parent = parent.parent
if (
parent?.type === "VariableDeclaration" ||
parent?.type === "FunctionDeclaration"
) {
const references =
parent.type === "VariableDeclaration"
? getReferences(parent.declarations[0].id)
: parent.id
? getReferences(parent.id)
: []

for (const reference of references) {
if (reference.identifier?.parent?.type === "CallExpression") {
if (
!visitedCallExpressions.includes(reference.identifier.parent)
) {
visitedCallExpressions.push(reference.identifier.parent)
doLint(
visitedCallExpressions,
contextCallExpression,
reference.identifier?.parent,
)
}
}
}
} else if (parent?.type === "ExpressionStatement") {
if (parent.expression.type !== "CallExpression") {
report(contextCallExpression)
} else if (lifeCycleReferences.includes(parent.expression)) {
report(contextCallExpression)
}
}
Copy link
Member

@ota-meshi ota-meshi Jun 10, 2023

Choose a reason for hiding this comment

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

I think it's safer to check the VariableDeclarator. Also, I think we should check if the right hand side operand is a function expression.
Otherwise, the following cases will result in false positives.

<script>
  import { getContext, onMount } from "svelte"

  const foo = getContext("foo")

  onMount(() => {
    foo()
  })
</script>
Suggested change
parent = parent.parent
if (
parent?.type === "VariableDeclaration" ||
parent?.type === "FunctionDeclaration"
) {
const references =
parent.type === "VariableDeclaration"
? getReferences(parent.declarations[0].id)
: parent.id
? getReferences(parent.id)
: []
for (const reference of references) {
if (reference.identifier?.parent?.type === "CallExpression") {
if (
!visitedCallExpressions.includes(reference.identifier.parent)
) {
visitedCallExpressions.push(reference.identifier.parent)
doLint(
visitedCallExpressions,
contextCallExpression,
reference.identifier?.parent,
)
}
}
}
} else if (parent?.type === "ExpressionStatement") {
if (parent.expression.type !== "CallExpression") {
report(contextCallExpression)
} else if (lifeCycleReferences.includes(parent.expression)) {
report(contextCallExpression)
}
}
if (
(parent?.type === "VariableDeclarator" &&
parent.init &&
(parent.init.type === "FunctionExpression" ||
parent.init.type === "ArrowFunctionExpression") &&
isInsideOf(parent.init, currentNode)) ||
parent?.type === "FunctionDeclaration"
) {
const references = parent.id ? getReferences(parent.id) : []
for (const reference of references) {
if (reference.identifier?.parent?.type === "CallExpression") {
if (
!visitedCallExpressions.includes(reference.identifier.parent)
) {
visitedCallExpressions.push(reference.identifier.parent)
doLint(
visitedCallExpressions,
contextCallExpression,
reference.identifier?.parent,
)
}
}
}
} else if (parent?.type === "ExpressionStatement") {
if (parent.expression.type !== "CallExpression") {
report(contextCallExpression)
} else if (lifeCycleReferences.includes(parent.expression)) {
report(contextCallExpression)
}
}
parent = parent.parent

@ota-meshi ota-meshi marked this pull request as draft June 8, 2024 07:50
@ota-meshi
Copy link
Member

The repository has been migrated to a monorepo.
I'm marking this as a draft because I think it requires major changes.
If anyone wants to implement this, feel free to open another PR.

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.

New Rule: svelte/valid-context-access
2 participants