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

Prevent #1688 (and other accidental prettier breakages) #1689

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jan 23, 2025

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:

function P(t) {
  return t.length > 0;
}

via:

console.log((await import('./syntax-prod.js')).preprocess('<h1></h1>'));

and the VS Code JavaScript Debug Terminal

@NullVoxPopuli NullVoxPopuli marked this pull request as draft January 23, 2025 16:21
@NullVoxPopuli
Copy link
Contributor Author

is draft, because I've yet to reproduce the situation created by #1688

Copy link

github-actions bot commented Jan 23, 2025

This PRmain
Dev
582K └─┬ .
169K   ├── runtime
159K   ├── syntax
100K   ├── compiler
 58K   ├── opcode-compiler
 27K   ├── manager
 19K   ├── validator
 11K   ├── program
8.9K   ├── reference
7.2K   ├── destroyable
6.3K   ├── util
4.3K   ├── node
3.4K   ├── global-context
2.5K   ├── wire-format
1.0K   ├── vm
969B   ├── encoder
844B   ├── vm-babel-plugins
606B   └── owner
582K └─┬ .
169K   ├── runtime
159K   ├── syntax
100K   ├── compiler
 58K   ├── opcode-compiler
 27K   ├── manager
 19K   ├── validator
 11K   ├── program
8.9K   ├── reference
7.2K   ├── destroyable
6.3K   ├── util
4.3K   ├── node
3.4K   ├── global-context
2.5K   ├── wire-format
1.0K   ├── vm
969B   ├── encoder
844B   ├── vm-babel-plugins
606B   └── owner
Prod
230K └─┬ .
 69K   ├── syntax
 63K   ├── runtime
 48K   ├── compiler
 18K   ├── opcode-compiler
7.9K   ├── manager
4.8K   ├── program
4.0K   ├── validator
3.6K   ├── reference
2.4K   ├── util
2.1K   ├── node
1.6K   ├── wire-format
1.5K   ├── destroyable
737B   ├── vm
594B   ├── global-context
516B   ├── encoder
469B   ├── vm-babel-plugins
155B   └── owner
230K └─┬ .
 69K   ├── syntax
 63K   ├── runtime
 48K   ├── compiler
 18K   ├── opcode-compiler
7.9K   ├── manager
4.8K   ├── program
4.0K   ├── validator
3.6K   ├── reference
2.4K   ├── util
2.1K   ├── node
1.6K   ├── wire-format
1.5K   ├── destroyable
737B   ├── vm
594B   ├── global-context
516B   ├── encoder
469B   ├── vm-babel-plugins
155B   └── owner

@@ -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",
Copy link
Contributor Author

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

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review January 23, 2025 19:58
package.json Outdated Show resolved Hide resolved
* copy of @glimmer/syntax, or else prettier brings its own already published
* copy of @glimmer/syntax
*/
describe('Prettier', () => {
Copy link
Contributor

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.

@@ -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> {
Copy link
Contributor

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?

Copy link
Contributor Author

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);
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@glimmer/syntax 0.94.0 is broken
2 participants