-
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
Prevent #1688 (and other accidental prettier breakages) #1689
base: main
Are you sure you want to change the base?
Conversation
is draft, because I've yet to reproduce the situation created by #1688 |
|
@@ -37,7 +37,7 @@ | |||
"test:babel-plugins": "yarn workspace @glimmer/vm-babel-plugins test", | |||
"test:browserstack": "ember test --test-port=7774 --host 127.0.0.1 --config-file=testem-browserstack.js", | |||
"test:lint": "eslint . --quiet", | |||
"test:node": "node bin/run-node-tests.mjs", |
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.
nothing was calling test:node
prior to this PR, and run-node-tests called global ember
, which isn't used in CI
d506053
to
41c9a7b
Compare
packages/@glimmer-workspace/integration-node-tests/test/prettier.test.ts
Outdated
Show resolved
Hide resolved
* copy of @glimmer/syntax, or else prettier brings its own already published | ||
* copy of @glimmer/syntax | ||
*/ | ||
describe('Prettier', () => { |
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.
Love having formal prettier tests.
packages/@glimmer-workspace/integration-node-tests/test/prettier.test.ts
Outdated
Show resolved
Hide resolved
packages/@glimmer-workspace/integration-node-tests/test/prettier.test.ts
Outdated
Show resolved
Hide resolved
packages/@glimmer-workspace/integration-node-tests/test/prettier.test.ts
Outdated
Show resolved
Hide resolved
@@ -14,8 +14,8 @@ export function assertPresent<T>(value: T, message?: string): asserts value is P | |||
} | |||
} | |||
|
|||
export function isPresentArray<T>(list: readonly T[]): list is PresentArray<T> { | |||
return list.length > 0; | |||
export function isPresentArray<T>(list?: readonly T[]): list is PresentArray<T> { |
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.
Hm... what uses isPresentArray
with an optional array?
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.
nothing -- it's a minification bug!
input:
private matchFor(
left: OffsetKind,
right: OffsetKind
): (left: PositionData, right: PositionData) => Out {
const nesteds = this._whens.match(left);
localAssert(
isPresentArray(nesteds),
`no match defined for (${left}, ${right}) and no AnyMatch defined either`
);
const callback = new WhenList(nesteds).first(right);
localAssert(
callback !== null,
`no match defined for (${left}, ${right}) and no AnyMatch defined either`
);
return callback;
}
output:
matchFor(t, e) {
const s = this._whens.match(t);
return P(), new I(s).first(e);
}
Fixes: #1688
This repo is going through a lot of infra changes, so because we haven't had any integration tests with prettier (since they consume
@glimmer/syntax
for hbs and glimmer parsing), we've accidentally broke them now 2 or 3 times recently 😬Takes the same approach as:
Where we use our real build in a real project outside the monorepo.
Every shortcut we take to make in-repo development easier needs to have extra testing to ensure we don't mess up publishing.
Both ember-data/warp-drive and glimmer-vm are not testing the code they ship to NPM.
In debugging, I copied the prod index.js and formatted it with prettier so that I could debug it:
via:
and the VS Code JavaScript Debug Terminal