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

5101: auth.ts: Update eslint-disable-next-line statement to be compatible with js files too #6037

Merged
merged 4 commits into from
Jul 26, 2022

Conversation

standuprey
Copy link
Contributor

This is a simple solution to the problem. There might be some difference between eslint's no-unused-var and @typescript-eslint's no-unused-var but it seems like it solves this problem.

If we're worried about those differences, another approach could be to transform the js files and remove @typescript-eslint/ as part of the transform. Could be overkill and have side effects

…line statement to be compatible with js files too
@nx-cloud
Copy link

nx-cloud bot commented Jul 25, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 3c9ccfc. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Jul 25, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 3c9ccfc
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62e026219d4a640008c46616

@jtoar jtoar added the release:fix This PR is a fix label Jul 26, 2022
@jtoar jtoar assigned jtoar and unassigned Tobbe Jul 26, 2022
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Hey @standup75 👋

This is a simple solution to the problem

You're talking about is this in newly-created JS projects right?

image

The reason you're seeing a CI failure here is that we lint the template as a part of CI, and the template is in TS. Since the inline-config comment for @typescript-eslint/no-unused-var rule is gone now, lint errors out. So we need that rule there if we want CI to pass. But I think we can eat our cake and have it too if we just include both rules (see my suggestion).

If we're worried about those differences, another approach could be to transform the js files and remove @typescript-eslint/ as part of the transform. Could be overkill and have side effects

We actually should have a legit step for this as there's a few other transforms we could make. But this is a much larger task.

packages/create-redwood-app/template/api/src/lib/auth.ts Outdated Show resolved Hide resolved
@jtoar
Copy link
Contributor

jtoar commented Jul 26, 2022

Ah I missed that this was linked to #6037—understood now!

@jtoar jtoar linked an issue Jul 26, 2022 that may be closed by this pull request
Stan Duprey and others added 2 commits July 26, 2022 10:35
@standuprey standuprey requested a review from jtoar July 26, 2022 17:36
@jtoar jtoar merged commit 3fe7dca into redwoodjs:main Jul 26, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jul 26, 2022
@Tobbe Tobbe changed the title 5101: Removed @typescript-eslint/ plugin name in eslint-disable-next-line statement to be compatible with js files too 5101: auth.ts: Update eslint-disable-next-line statement to be compatible with js files too Aug 5, 2022
@jtoar jtoar modified the milestones: next-release, v3.0.0 Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ESLint error on roles param in JS project
3 participants