-
Notifications
You must be signed in to change notification settings - Fork 192
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
Refresh tsconfig and eslint setup #1677
Conversation
@@ -1,3 +1,12 @@ | |||
{ | |||
"recommendations": ["foxundermoon.shell-format"] | |||
"recommendations": [ |
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.
These recommendations are documented in guides/workspace/vscode.md
"fontWeight": "bold", | ||
"color": "#ff0000" | ||
} | ||
"editor.rulers": [ |
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.
Duplicates .editorconfig, but necessary because of
editorconfig/editorconfig#89.
Rulers are used to power rewrap
, which helps wrap
documentation comments, which are not currently enforced
by prettier or eslint. It would be difficult to fully
automate wrapping (although it's worth trying in the)
future, and rewrap
works well enough by requiring the
author to manually invoke rewrap, but then automating
the wrapping itself.
It's a tradeoff, but it's good enough for now.
"editor.defaultFormatter": "esbenp.prettier-vscode", | ||
"editor.formatOnSave": false | ||
}, | ||
"[json][jsonc][markdown][yaml]": { |
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.
Same deal with JSON. Note that JSON and JSONC are linted via eslint
(via eslint-plugin-jsonc
) in
this repository, so fixAll
is meaningful.
"**/dist": true, | ||
"tracerbench-results": true | ||
}, | ||
"files.insertFinalNewline": true, |
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.
These already come from .editorconfig
] | ||
}, | ||
"inline-bookmarks.view.showVisibleFilesOnly": true, | ||
"javascript.preferences.importModuleSpecifier": "project-relative", |
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.
auto-import from other workspace packages directly (i.e. from "@glimmer/vm"
) rather than importing
(incorrectly) from a relative path (which would escape the package boundary).
@@ -0,0 +1,4 @@ | |||
import type { SerializedTemplateWithLazyBlock } from '@glimmer/interfaces'; |
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.
There are two changes in all files in this directory:
- Importing from
@glimmer/interfaces
rather than a relative path. - Using
.d.hbs.ts
, which is the new TypeScript default forallowArbitraryExtensions
(see the docs)
@@ -1,6 +1,9 @@ | |||
import { swapRows, type Item, updateData, buildData } from '@/utils/data'; |
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.
Throughout the benchmarks, use subpath
imports (the new npm standard) rather than
path mappings.
@@ -1,3 +1,4 @@ | |||
/* eslint-disable @typescript-eslint/restrict-plus-operands */ |
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.
The biggest change across the entire PR is the addition of eslint-disable
directives to account
for the more consistent application of eslint rules in this PR.
@@ -1,13 +0,0 @@ | |||
/** @type {import("eslint").Linter.Config} */ |
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.
We don't need individual .eslintrc files anymore.
@@ -45,7 +46,9 @@ for (let filePath of files) { | |||
let content = file.toString(); | |||
|
|||
for (let searchFor of FORBIDDEN) { | |||
if (content.match(searchFor)) { | |||
const match = typeof searchFor === 'string' ? content === searchFor : searchFor.test(content); |
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.
Bug (?) caught by linting. Even if it somehow works, it seems good to make the code match the types.
@@ -1,12 +1,14 @@ | |||
import { readFile, writeFile, unlink } from 'node:fs/promises'; | |||
import { writeFileSync } from 'node:fs'; |
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.
The PR has a lot of changes to imports to make them consistent.
@@ -55,7 +57,7 @@ const unpack = pkgs.map(async (pkg) => { | |||
const packageJsonPath = join(pkgDest, 'package.json'); | |||
const packageJson = JSON.parse(await readFile(packageJsonPath, { encoding: 'utf8' })); | |||
delete packageJson.devDependencies; | |||
await writeFileSync(packageJsonPath, JSON.stringify(packageJson, null, 2), { | |||
writeFileSync(packageJsonPath, JSON.stringify(packageJson, null, 2), { |
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.
writeFileSync
is not async, so no need for await
. There are a number of cases of this in the PR.
@@ -16,11 +16,11 @@ const ALL: (string | null)[] = [ | |||
const TYPES = ['interface', 'code', 'debug', 'all'] as const; | |||
|
|||
if (emitter.options.help) { | |||
throw usage(); | |||
usage(); |
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.
An old version of TS didn't properly handle functions returning never
, and we worked around that
by throwing the never
. But now that's fixed and it's a lint error to throw never
, so there are a
number of places where this is fixed.
@@ -1,11 +1,23 @@ | |||
{ | |||
"name": "@glimmer-workspace/bin", | |||
"type": "module", |
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.
All packages have "type": "module"
to reflect reality.
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.
this means we can start using publint and arethetypeswrong to ensure we don't have publishing mishaps!
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.
yay
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.
Yes exactly. This PR also sets up publint (for all published packages) and publint should be clean
right now.
"private": true, | ||
"repo-meta": { |
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.
This PR introduces a way of specifying package-specific tweaks to the overall linting
configuration. The details are documented in @glimmer-workspace/eslint-plugin/README.md
bin/package.json
Outdated
"scripts": { | ||
"test:lint": "eslint .", | ||
"test:lint": "eslint . --quiet", |
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.
This PR uses warning
level lints for situations where we want to annotate a line with a disable
as documentation for the future (when we enable stricter lints), but don't want the lint to fail
yet. It's a temporary measure, and we should switch back to error-only lints once we've moved
everything to stricter lints.
} finally { | ||
if (process.env['GITHUB_RUN_ID']) { | ||
await run('ember', ['browserstack:results']); | ||
} | ||
await run('ember', ['browserstack:disconnect']); | ||
// eslint-disable-next-line n/no-process-exit | ||
process.exit(0); |
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.
The previous code ran process.exit() in the try
, which would mean that the finally
would never
run. I'm not sure why that worked (perhaps because we never relied on browserstack to run
properly?), but I moved the exit
to the end of the finally
rather than completely delete it,
since it presumably exists for a reason?
await /** @type {Promise<void>} */ ( | ||
new Promise(async (fulfill, reject) => { | ||
const page = await browser.newPage(); | ||
const page = await browser.newPage(); |
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.
This code is adjusted to run the asynchronous part of the work outside of the Promise
constructor.
While keeping it inside the Promise
constructor works in this situation, it doesn't behave the
way it looks like it does.
bin/run-tests.mjs
Outdated
}) | ||
); | ||
|
||
await page.goto('http://localhost:60173?hidepassed&ci'); |
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.
This is a change in behavior (the old code was fire-and-forget because of the confusing way it used
the Promise
constructor). This is my best guess, and we need to verify that it still works
correctly.
In general, there's a bunch of "fire and forget" code in the bin
directory, and we should document
when we do that, and why.
@@ -1,7 +1,12 @@ | |||
import 'zx/globals'; |
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.
Why import globals when you can just import $
?
@@ -1,23 +0,0 @@ | |||
{ |
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.
No more project references.
@@ -38,7 +38,7 @@ import { | |||
SPLAT_HEAD, | |||
THIS_VAR, | |||
} from '@glimmer/constants'; | |||
import { assert, exhausted, expect, isPresentArray } from '@glimmer/debug-util'; | |||
import { exhausted, expect, isPresentArray,localAssert } from '@glimmer/debug-util'; |
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.
Renamed assert
to localAssert
to make it super-clear that it's not included in the build output.
@@ -67,7 +67,7 @@ function assertIfUnlessInlineKeyword(type: string) { | |||
inverted ? 'false' : 'true' | |||
}, and 3. the value to return if the condition is ${ | |||
inverted ? 'true' : 'false' | |||
}. Received ${positional?.size ?? 0} parameters`, | |||
}. Received ${positional.size} parameters`, |
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.
There's quite a bit of code in the repo that checked for things that can't happen. Sometimes, this
is because it's handling code from untyped JS users. In other cases, like this one, it's purely
internal code interacting with other typed code, and the check is a holdover from a previous time
when the check was necessary.
The current linting setup warns about situations like this. In some situations, like this one, I removed the unnecessary
check. In other situations, I disabled the lint and identified the fact that the check is for JS callers.
@@ -8,8 +8,8 @@ import type { NormalizationState } from '../context'; | |||
import { Err } from '../../../shared/result'; | |||
|
|||
export interface KeywordDelegate<Match extends KeywordMatch, V, Out> { | |||
assert(options: Match, state: NormalizationState): Result<V>; | |||
translate(options: { node: Match; state: NormalizationState }, param: V): Result<Out>; | |||
assert: (options: Match, state: NormalizationState) => Result<V>; |
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.
Since assert
and translate
are extracted, this documents that they are arrows without a this
binding.
@@ -115,20 +115,9 @@ export type KeywordNode = | |||
| ASTv2.InvokeBlock | |||
| ASTv2.ElementModifier; | |||
|
|||
export function keyword< |
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.
This code is only used in one place and abstracts very little, and the bulk of the implementation is
type annotations. ✂️
@@ -29,7 +29,7 @@ export function describeOp( | |||
program: Program, | |||
meta: Nullable<BlockMetadata> | |||
): Fragment { | |||
const { name, params } = debugOp(program, op, meta)!; | |||
const { name, params } = debugOp(program, op, meta); |
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.
There's a decent amount of totally unnecessary !
assertions, and they were removed via autofix.
@@ -30,31 +30,31 @@ export class Fragment<T extends FragmentType = FragmentType> { | |||
static integer( | |||
this: void, | |||
value: number, | |||
options?: Omit<IntegerFragment, 'kind' | 'value'> | undefined | |||
options?: Omit<IntegerFragment, 'kind' | 'value'> |
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.
You don't need | undefined
in parameter positions, and this (and many other cases like it) were
caught by the new lint rules.
@@ -53,12 +53,10 @@ export class DebugLogger { | |||
#lines(type: 'log' | 'debug' | 'group' | 'groupCollapsed', lines: FlushedLines): void { | |||
const [first, ...rest] = lines; | |||
|
|||
if (first) { |
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.
The whole point of FlushedLines
is to require at least one lint, so this check is unnecessary. ✂️
@@ -172,6 +173,7 @@ export class VmSnapshotArrayDiff<N extends string | number, T extends unknown[]> | |||
post = value(after, { ref: `${i}:push` }); | |||
} else if (op === 'retain') { | |||
post = value(after, { ref: `${i}:after` }); | |||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- exhaustion check |
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.
There's possibly a better way to do this, but the issue is: the linter knows that there's only one
kind of op
left ('pop'
) so it thinks that the check is unnecessary (and could be handled by a
simple else
). However, it's here to ensure that if op
gets a new member, the type will fail and
force us to add a new clause.
There are a few places like this, and it would be good to consolidate on a single good solution, but
I think ad-hoc solutions are fine for now, since there aren't a ton of these cases, and the "no
unnecessary check" rule is quite valuable and caught a lot of places where the extra check was
either a mistake or caused confusion.
@@ -66,9 +70,8 @@ export function describeOpcode(type: string, params: Dict<SomeDisassembledOperan | |||
} | |||
} | |||
|
|||
function stringify(value: number, type: 'constant'): string; | |||
function stringify(value: number, type: 'constant' | 'variable' | 'pc'): string; |
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.
I generally took the linter's advice and consolidated overloads when possible.
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.
Some minor things left:
- conflicts
- benchmark doesn't run
- Refresh tsconfig and eslint setup #1677 (comment) -- looks like we want to use either
/u
or no flag for runtime regex if we don't need to explicitly be matching for unicode (unicode characters aren't valid elements, ya?) - leftover commented out code: Refresh tsconfig and eslint setup #1677 (comment)
dfa6c76
to
2b8b1f6
Compare
|
c3c99c6
to
b936364
Compare
Current performance boost on my lil'linux laptop:
this is very nice! I wonder how much has to do with there just being less code in the output now |
036513e
to
e12d2f4
Compare
8d00ff7
to
8b424e6
Compare
1. Migrate to ESLint 9 + new config format 2. Migrate to a single tsconfig file for the entire repo In general, this results in more consistency across the repo, including in one-off packages and tests, but it still gives us flexibility to create special rules if necessary. As part of this process, a new `repo-metadata` package was created that allows other infrastructure packages to get workspace metadata. This package has a static JSON file with the information about each package in the workspace. This was important in the current PR because it allows our eslint config to be sensitive to information in individual packages without slowing down the boot time of eslint. Fully migrating to ESLint 9 required: - Making the whole repo clean with `eslint .` - Making `eslint .` in individual packages work (and perhaps move the custom configuration to configs in each package - Moving the guts of the code into `@glimmer-workspace/eslint-plugin`. This should be **much** more straightforward with ESLint 9. - Porting all of the ESLint rules from the current setup to the new config, or deciding to ignore them. - Updating our JSON linting Rather than using a workspace package for benchmarks and trying to get `vite build` to emulate the expected behavior, we `pack` the dependencies and refer to them directly in the `package.json`. This is a little bit more involved than it might seem like it should be because the benchmark has to run on the `main` branch, so it has to be very general and can't use any infrastructure in the workspace itself that could affect the builds. On the bright side, that constraint pushes us towards treating the benchmark environment as a normal consumer of the normal Glimmer packages, which is good. The only tiny exception to that rule is that the build script builds the `@glimmer-workspace/benchmark-env` package, even though it's not a public package. This means that the code in `@glimmer-workspace/benchmark-env` needs to make sure it can be built. The current setup allows us to use the `main` branch's `@glimmer-workspace/benchmark-env` package with the experiment's `krausest` package, so it seems reasonable to build the benchmark env and use it as a traditional package rather than trying to get two workspaces to somehow mesh together (which is what got us into trouble in the first place). Note: Getting this working right now requires a little bit of hackery when setting up the current `control` (i.e. `main`) branch, because `main` isn't set up to build the `@glimmer-workspace/benchmark-env`. Once this PR is merged, we should be able to remove those hacks and rely on vanilla package builds for the entire setup going forward. Other changes: - Document recommendations - Use LFS - Explain recommended extensions for vscode - Remove unused dependencies NOTE: This PR comes after multiple attempts to set up a `.meta-updater` config, but I realized that it would be much easier after a long-needed overhaul to the tsconfig and eslint configs.
- `meta-updater` now automates the process of keeping packages aligned with `publint` requirements based on whether they're published. - add back a `-f` that was accidentally removed from the bench setup - Created top-level `repo:*` scripts for `turbo` commands so that there's no confusion between the repo-wide commands and the same-named commands in packages. - Added `@bandaid` highlighting rule to capture situations where code exists as a temporary workaround, and allows you to document the reason for the workaround so it can be identified and cleaned up later. - Update GitHub actions to align with the new `turbo` and repo-wide workflows. - Improve docs in `workspace-management.md` to describe the new turbo workflows and repo-wide commands. The tests were failing because (apparently) vite wasn't analyzing the `index.html` file properly. This resulted in multiple copies of the validator code getting included in the tests (via chunks and via the `.ts` module itself). To make matters worse, the validator code included in the chunk was missing the code that detects duplicates, so the only failure symptom was an error reporting that the `COMPUTE` symbol wasn't present on an instance of `Tag`. Being explicit about entry points fixed the problem and made the tests pass. I don't know why this is necessary (surely vite should undertand its own import.meta.glob), but it's a relatively small change, and I think we can live with it for the moment.
8b424e6
to
d60ce64
Compare
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.
Let's gooooo
"type": "module", | ||
"exports": "./index.ts", | ||
"publishConfig": { | ||
"access": "public", | ||
"exports": { | ||
".": { | ||
"require": { |
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.
removing require here breaks prettier.
let's fix in a followup though
This is a significant overhaul of the configuration setup for the repository, driven largely by the work required to update the repository to flat config and ESLint 9.
In general, this results in more consistency across the repo, including in one-off packages and tests, but it still gives us flexibility to create special rules if necessary.
As part of this process, a new
repo-metadata
package was created that allows other infrastructure packages to get workspace metadata. This package has a static JSON file with the information about each package in the workspace. This was important in the current PR because it allows our eslint config to be sensitive to information in individual packageswithout slowing down the boot time of eslint.
Fully migrating to ESLint 9 required:
eslint .
eslint .
in individual packages work (and perhaps move the custom configuration to configs in each package@glimmer-workspace/eslint-plugin
. This should be much more straightforward with ESLint 9.Increased Benchmark Isolation and Predictability
Rather than using a workspace package for benchmarks and trying to get
vite build
to emulate the expected behavior, wepack
the dependencies and refer to them directly in thepackage.json
.This is a little bit more involved than it might seem like it should be because the benchmark has to run on the
main
branch, so it has to be very general and can't use any infrastructure in the workspace itself that could affect the builds.On the bright side, that constraint pushes us towards treating the benchmark environment as a normal consumer of the normal Glimmer packages, which is good.
The only tiny exception to that rule is that the build script builds the
@glimmer-workspace/benchmark-env
package, even though it's not a public package. This means that the code in@glimmer-workspace/benchmark-env
needs to make sure it can be built.The current setup allows us to use the
main
branch's@glimmer-workspace/benchmark-env
package with the experiment'skrausest
package, so it seems reasonable to build the benchmark env and use it as a traditional package rather than trying to get twoworkspaces to somehow mesh together (which is what got us into trouble in the first place).
Note
Getting this working right now requires a little bit of hackery when setting up the current
control
(i.e.main
) branch, becausemain
isn't set up to build the@glimmer-workspace/benchmark-env
.Once this PR is merged, we should be able to remove those hacks and rely on vanilla package builds for the entire setup going forward.
Meta Updater and Repo Scripts
This PR introduces a meta-updater setup that keeps packages aligned with repository conventions based upon whether they're published, need a build, are test packages, etc.
It further standardized per-package scripts via
turbo
, and created top-levelrepo:*
scripts to differentiate repo workflows from standard scripts in each package. This PR also begins to document repo workflows (inguides/workspace
).Inline Notes
This PR adds a
@bandaid
highlighting rule to capture situations where code exists as a temporary workaround. The rule and allows you to document the reason for the workaround so it can be identified and cleaned up later.I also made more use of an existing
@fixme
annotation.Testing Entry Points
While developing this PR, the tests began to fail because Vite was failing to interpret the
import.meta.glob
s inindex.html
file as entry points. This resulted in multiple copies of the validator code getting included in the tests (via chunks and via the.ts
module itself).To make matters worse, the validator code included in the chunk was missing the code that detects duplicates, so the only failure symptom was an error reporting that the
COMPUTE
symbol wasn't present on an instance ofTag
.Being explicit about entry points fixed the problem and made the tests pass. I don't know why Vite doesn't interpret
import.meta.glob
properly in this situation, but I think we can live with duplicating the test glob invite.config.mjs
for now.Other Improvements