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

next/inline-script-id does not handle JSXSpreadAttributes correctly #34030

Closed
no-yan opened this issue Feb 6, 2022 · 4 comments · Fixed by #34473
Closed

next/inline-script-id does not handle JSXSpreadAttributes correctly #34030

no-yan opened this issue Feb 6, 2022 · 4 comments · Fixed by #34473
Labels
good first issue Easy to fix issues, good for newcomers Linting Related to `next lint` or ESLint with Next.js.

Comments

@no-yan
Copy link

no-yan commented Feb 6, 2022

Run next info (available from version 12.0.8 and up)

No response

What version of Next.js are you using?

canary

What version of Node.js are you using?

16.13.0

What browser are you using?

no

What operating system are you using?

Ubuntu 20.04.3 LTS

How are you deploying your application?

no

Describe the Bug

In response to comment: #32421 (comment)

An error occurs because the rule next/inline-script-id does not handle JSXSpreadAttribute correctly.
After #32178, It may still give an error when there is a JSXSpreadAttribute.
JSXSpreadAttribute can be any AssignmentExpression (Spec) , but inline-script-id only supports special cases.
That is based on the assumption, every JSXSpreadAttribute have property key.name.

Source:

} else if (attribute.type === 'JSXSpreadAttribute') {
attribute.argument.properties.forEach((property) => {
attributeNames.add(property.key.name)

When a SpreadAttribute is passed, it will be a variable in most cases. If so, Eslint cannot find the property name.
To support other cases, we can build some complex logic, but I don't think it is worth it.
Can't we simply ignore the JSXSpreadAttribute?

Expected Behavior

Rules should not report error when JSXSpreadAttribute is exist.

To Reproduce

The failing case I checked is the following code.

{
  code: `import Script from 'next/script';
  const spread = { strategy: "lazyOnload" }
  export default function TestPage() {
    return (
      <Script {...spread} id={"test-script"}>
        {\`console.log('Hello world');\`}
      </Script>
    )
  }`,
},

You can confirm AST of this by using https://astexplorer.net/ .
@stefanprobst

@no-yan no-yan added the bug Issue was opened via the bug report template. label Feb 6, 2022
@no-yan
Copy link
Author

no-yan commented Feb 6, 2022

I would like to implement this, but I would like to hear your opinion first.

@ijjk
Copy link
Member

ijjk commented Feb 6, 2022

Hi, thanks for looking into this, yeah if the spread attribute is not able to be checked for the values we should definitely ignore it in that case instead of showing an error, a PR updating this would be welcome!

@balazsorban44 balazsorban44 added Linting Related to `next lint` or ESLint with Next.js. good first issue Easy to fix issues, good for newcomers and removed bug Issue was opened via the bug report template. labels Feb 6, 2022
@rohan-kulkarni-25
Copy link

Hey @ijjk !! I will like to work on this issue can you please guide me on this !!

SukkaW added a commit to SukkaW/next.js that referenced this issue Feb 17, 2022
@kodiakhq kodiakhq bot closed this as completed in #34473 Feb 25, 2022
kodiakhq bot pushed a commit that referenced this issue Feb 25, 2022
## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

Fixes #34030.

The PR is still WIP as the test case hasn't been added, help or change is welcome.

cc @no-ya @ijjk 

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers Linting Related to `next lint` or ESLint with Next.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants