fix: configure ESLint environments and resolve CI lint errors#89
fix: configure ESLint environments and resolve CI lint errors#89shishir-21 wants to merge 12 commits intogbowne1:masterfrom
Conversation
|
Hi @gbowne1 👋 I’ve resolved all ESLint environment and no-undef errors (Node / Service Worker globals), and npm run lint now passes with warnings only. The remaining CI failures are due to no-console warnings being treated as blocking in CI. There are no functional or syntax issues left. Before proceeding further, I wanted to confirm your preference: Happy to adjust based on your guidance. Thanks! |
|
@gbowne1 |
|
Hey I resolved the head ref issue with #84 so it's ready to be merged in my opinion. @shishir-21 , PR #84 resolves the trailing colon issue. |
|
Thanks for the clarification 👍 |
|
Let's go back and re review all open PRs especially the ones which got rebased. I have a funny feeling that if we don't #78 soon were going to keep have this same issue |
gbowne1
left a comment
There was a problem hiding this comment.
@shishir-21
I checked out this change locally for testing and review for merge
This correctly addressed most of the CI issues as far as I can tell.
Approving this PR for merge and merging pending merge of the .env issue PR #84 and further approved review by collaborators and maintainers.
Let's look at doing #78 very soon.
Thanks for the opportunity to review your PR and for your contribution to this project
|
I would also review the pr by @glenjaysondmello #87 as it seems to cause the CI to go green 👀 |
|
@shishir-21 @Ved178 @glenjaysondmello @abhisheksingh1204 I merged the fixes. Another rebase? The latest merge as of today should fix the CI. Let's try and get in the last of the PRz and re review. We will need the PR adding going live and chat before we start any refactoring |
Ved178
left a comment
There was a problem hiding this comment.
Approving this, changes look good and I think it should resolve the lint errors.
|
The run shown below failed due to missing and or dependency not installed during the run. I think this is an artifact. The package-lock got fixed in a recent merge. Maybe a rebase? Even though it shows no merge conflicts. |
|
I think we could Run npm install locally (not npm ci) Commit the changes to package-lock.json.Push the changes This is due to missing items from package.json not ending up in package-lock.json |
|
@gbowne1 I’ll run npm install locally (not npm ci) to regenerate the package-lock.json, commit the updated lockfile, and push the changes to fix the dependency mismatch. I’ll update the PR shortly and re-trigger CI. |
|
Thanks. package-lock.json in version control is kinda dangerous especially if multiple contributors touch it. I prefer keeping package-lock.json local only and ignored same with node_modules (they both get created on install anyway). There's pros and cons to commit them to the repo. |
|
@gbowne1 Since package-lock.json is not intended to be version controlled, it looks like CI might still be using npm ci, which requires the lockfile. Should we update the CI workflow to use npm install instead of npm ci? |
|
I think so. But again there's caveats to both ways. I guess that we just see where the dust settles. I'd leave it the way it is now till we can get to a clean CI with what we have right now |
94d31a3 to
1bc5e3a
Compare
|
The CI is currently failing because it runs npm ci, which requires a committed package-lock.json. Since package-lock.json is not version controlled, npm ci will continue to fail. We’ll need to either: 1.) Commit package-lock.json, or 2.) Update CI to use npm install instead of npm ci. Let me know which direction you'd prefer and I’ll update accordingly. |
|
Well, let's stick with npm package_lock.json is in .gitignore though.. lines 34 => 36 |
|
Since package-lock.json is currently ignored in .gitignore, npm ci cannot work because it requires a committed lockfile. To keep the lockfile ignored, the CI workflow would need to use npm install instead of npm ci. Otherwise, we would need to remove package-lock.json from .gitignore and commit it. Let me know which approach you'd like and I’ll update accordingly. |
|
Well, it still keeps getting committed and/or changed even though it's been ignored by .gitignore.. So 🤷♂️ I'd like to remove it from .gitignore L# 34=>36 then recommit it and whatever other fixes needed so that what we have now works properly. |
|
Resolved all ESLint and Prettier issues. CI pipeline now passes successfully. |
|
Yes looks like everything passed. Hopefully it stays this way.. but maybe we merge this next in line? |
|
Thanks! Yes, all lint and CI issues are resolved. |
|
Well.. 🤷♂️ Another merge broke this yet again. Had more merge conflicts which I resolved Now the CI here fails again |
46c9229 to
e8636c9
Compare
|
Closing this PR due to complex rebase conflicts. |
What was fixed
no-undeferrors (process,clients)Result
npm run lintnow passes (warnings only, no errors)No functional changes.