-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Add ESLint to the build #1390
Add ESLint to the build #1390
Conversation
Automated comment from CodeApprove ➜⏳ @jotaen4tinypilot please review this Pull Request |
This reverts commit cb5493f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
⏳ Approval Pending (4 unresolved comments)
Approval will be granted automatically when all comments are resolved
In: app/static/js/app.js:
> Line 9
import { isPasteOverlayShowing, showPasteOverlay } from "./paste.js";
Note that we have a duplicate import of the paste.js
file now. We also import it as plain, global script in index.html
. However, due to the new export
keywords in paste.js
, the file can only be imported in an ESModule context, not in a plain script context anymore. Therefore, it currently produces a console error in the Browser, caused by the <script src>
-style import:
The execution context of app.js
is an ESModule, so in this context, the export
/import
is safe and supported.
Long story short: I think your proposed change is the right way forward, and we can just abandon <script src="/js/paste.js"></script>
to get rid of the interpreter error.
As a follow up, we could consider to refactor paste.js
, and make the encapsulation a little nicer (e.g. avoid the global document
object access). In order to avoid redundant work, this could be part of #1137, where we’d touch that code anyways.
In: app/static/js/app.js:
> Line 13
// Suppress ESLint warnings about undefined variables.
I’m wondering whether we should make this comment more specific. What do you think about giving a hint where the global io
variable is coming from, like who defines it, and what it means. Along the lines of:
// `io` is defined by the Socket.IO library, which is globally available on the page.
/* global io */
(ditto throughout)
In: dev-scripts/build-javascript:
> Line 20
./dev-scripts/lint-frontend
This is super nitty (and mostly stylistic + optional), but to me it would feel slightly more natural/common to flip the order here, i.e. to first run the static code analysis (eslint), and then the unit tests (mocha).
In: dev-scripts/lint-frontend:
> Line 14
npm run lint
When running Prettier via the check-style
dev script, we invoke the Prettier script directly from our bash dev script. Shall we do the same thing here, to avoid one level of indirection?
To me it would make sense to either use npm scripts, or to use some sort of custom helper (like a bash script). In this case, however, we seem to do both: we have a custom helper script, which invokes an npm script, which invokes the actual thing. So I’m thinking that instead of npm run lint
, we could directly do:
./node_modules/.bin/eslint './**/*.js' './**/*.html'
As an aside, I personally never use npm scripts, because the only benefit which that really has is that you don’t have to prepend node_modules/.bin/
to any installed npm binaries. On the flip side, you have to maintain bash commands within a single-line JSON string, and the JSON file format doesn’t allow you to add comment annotations. Therefore, I’d consider the usage of npm scripts an overall net loss.
In: package.json:
> Line 7
"eslint": "8.40.0",
I realized that we don’t have a package-lock.json
in our repo, but I believe we should have that: #1393
If we’d go forward with that, we either need to update this PR or the other one, depending on the order in which we merge (since we add a new dev dependency here).
👀 @mtlynch it's your turn please take a look
While reviewing #1390, I realized that we haven’t checked in `package-lock.json` into version control, but we explicitly ignore it currently. [`package-lock.json` is an auto-generated manifest file](https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json) which is created and maintained by `npm` on every change to `package.json`. [Having the lock file under version control is actually recommended](https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#:~:text=This%20file%20is%20intended%20to%20be%20committed%20into%20source%20repositories), because in contrast to the pure `package.json` file, the lock file captures the entire dependency tree. This aims to make builds more reproducible, faster, and safer (due to checksums). Note: if git commit statistics are important to us, then we can also commit this under a separate account (e.g. some sort of bot account), since `package-lock.json` changes are always relatively large diffs in terms of LOC added/deleted. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1393"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
⏳ Approval Pending (4 unresolved comments)
Approval will be granted automatically when all comments are resolved
In: package.json:
> Line 7
"eslint": "8.40.0",
Update: package-lock.json
is in git now, so we have to run npm install
on this branch again, and commit the resulting package-lock.json
changes.
👀 @mtlynch it's your turn please take a look
@mtlynch which NodeJS version do you use? Because on CI, we use Node 14 + npm 6, which generates a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
In: app/static/js/app.js:
> Line 9
import { isPasteOverlayShowing, showPasteOverlay } from "./paste.js";
Ah, fixed.
In: app/static/js/app.js:
> Line 13
// Suppress ESLint warnings about undefined variables.
Good idea, added. I left in the note about ESLint, too, since readers might not understand what we're doing otherwise.
In: dev-scripts/build-javascript:
Sure, fixed.
In: dev-scripts/lint-frontend:
Oh, that's true. I agree with skipping npm scripts.
In: package.json:
> Line 4
"eslint": "8.40.0",
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
Approved: I have approved this change on CodeApprove and all of my comments have been resolved.
Yeah, I was using node 16 + npm 8. I've switched back to preserve the lockfile version. |
This adds ESLint to the build. I'm surprised we didn't already have it! I thought it was there all along.
This also makes a few changes in response to issues ESLint flagged. I tried to keep the fixes limited to very low-risk changes. For anything more substantial, I just suppressed the rule for that line.
A few are worth calling out:
tinypilot/app/static/js/app.js
Lines 75 to 77 in 9beae18
This just never worked. It was introduced in #326 and never referred to anything. I think I just copy/pasted from the section where
resolve
was defined and didn't realize it wasn't defined in this context.I've replaced it with a
return;
.tinypilot/app/templates/custom-elements/remote-screen.html
Lines 174 to 182 in 9beae18
This has been broken since #328 and nobody seems to have noticed, so I've just removed this code.