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

Merged
merged 29 commits into from
Jan 26, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
bcf1cd3
Try to make a test to prevent #1688 (and other prettier breakages)
NullVoxPopuli Jan 23, 2025
07347e6
Seems fine so far...
NullVoxPopuli Jan 23, 2025
67841dc
Add node testing to CI
NullVoxPopuli Jan 23, 2025
0e3dde1
delete unused file
NullVoxPopuli Jan 23, 2025
dcebdb4
Remove mention of run-node-tests
NullVoxPopuli Jan 23, 2025
42ee4d0
Why isn't linting working?
NullVoxPopuli Jan 23, 2025
4554a5a
As I feared, we have the build wrong
NullVoxPopuli Jan 23, 2025
85685a9
idk man
NullVoxPopuli Jan 23, 2025
b22bc20
run pnpm repo:update:metadata
NullVoxPopuli Jan 23, 2025
41c9a7b
Lints -- these are actually all correct
NullVoxPopuli Jan 23, 2025
e484f96
Fix #1688
NullVoxPopuli Jan 23, 2025
ba76cdb
Move test project into a real project, rather than tmp for easier pok…
NullVoxPopuli Jan 23, 2025
ddd5f2b
Set vitest conditions
NullVoxPopuli Jan 23, 2025
2d05f5a
Curious
NullVoxPopuli Jan 23, 2025
5de9010
Fix node smoke
NullVoxPopuli Jan 23, 2025
05069ee
Remove extraneous test
NullVoxPopuli Jan 23, 2025
c31c56d
Why doesn't my local CLI env match CI
NullVoxPopuli Jan 23, 2025
ebf3909
lockfile
NullVoxPopuli Jan 23, 2025
8074cd1
different logging
NullVoxPopuli Jan 23, 2025
b161bc5
Now its working?
NullVoxPopuli Jan 23, 2025
09d3fc0
Codify the requirements for running the types check
NullVoxPopuli Jan 24, 2025
3207df8
More turbo progress
NullVoxPopuli Jan 24, 2025
e6c337d
omg it worked
NullVoxPopuli Jan 24, 2025
1003ff5
Maybe I just remove the local lint script
NullVoxPopuli Jan 24, 2025
75061ca
Don't need that anymore
NullVoxPopuli Jan 24, 2025
5455815
Fix
NullVoxPopuli Jan 24, 2025
4a89ee9
Updates
NullVoxPopuli Jan 24, 2025
8206d97
fix
NullVoxPopuli Jan 24, 2025
3e14e23
last fix?
NullVoxPopuli Jan 24, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ jobs:
repo-token: ${{ secrets.GITHUB_TOKEN }}
- run: pnpm repo:lint:all


verify:
name: Verify
runs-on: ubuntu-latest
Expand All @@ -62,6 +63,37 @@ jobs:
- run: pnpm repo:update:metadata
- uses: wyvox/action-no-git-diff@v1


test-node:
name: Node
runs-on: ubuntu-latest
needs: ['install_dependencies']
timeout-minutes: 5

steps:
- uses: wyvox/action@v1
with:
node-version: 22.13.0
repo-token: ${{ secrets.GITHUB_TOKEN }}
- run: pnpm test:node

test-smoke:
name: Smoke
runs-on: ubuntu-latest
needs: ['install_dependencies']
timeout-minutes: 5

steps:
- uses: wyvox/action@v1
with:
node-version: 22.13.0
repo-token: ${{ secrets.GITHUB_TOKEN }}
- working-directory: ./smoke-tests/node
run: |
pnpm test:setup
pnpm test:node


test-chrome:
name: Chrome
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
.DS_Store
/dist
**/dist
/control-dist/
Expand Down
4 changes: 2 additions & 2 deletions .meta-updater/main.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,9 @@ export default () =>

const scripts = /** @type { JsonObject } */ (actual.scripts ??= {});

update(scripts, 'test:lint', 'eslint . --quiet');

// replaced with prepack
delete scripts['test:types'];
delete scripts['test:lint'];

const updateRepo = () => {
update(actual, 'repository', {
Expand Down Expand Up @@ -57,6 +56,7 @@ export default () =>
*/
if (isRoot) {
updateRepo();
update(scripts, 'test:lint', 'eslint . --quiet');
} else {
delete actual.repository;
}
Expand Down
4 changes: 1 addition & 3 deletions .meta-updater/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@
"keywords": [
"node"
],
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@pnpm/find-workspace-dir": "^1000.0.1",
"@pnpm/meta-updater": "^2.0.3",
Expand Down
4 changes: 1 addition & 3 deletions bin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@
"*"
]
},
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer-workspace/repo-metadata": "workspace:*",
"@pnpm/workspace.find-packages": "^1000.0.5",
Expand Down
46 changes: 0 additions & 46 deletions bin/run-node-tests.mjs

This file was deleted.

10 changes: 7 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,20 @@
"lint:format": "prettier -c .",
"postinstall": "node --disable-warning=ExperimentalWarning --experimental-strip-types ./bin/bench-packages.mts",
"repo:lint:all": "turbo run lint:all",
"repo:lint:files": "turbo run test:lint",
"repo:lint:files": "turbo run //#test:lint",
"repo:lint:fix": "turbo run test:lint -- --fix && prettier -w .",
"repo:lint:pub": "turbo run test:publint",
"repo:lint:types": "tsc -b",
"repo:lint:types": "turbo run //#test:types",
"repo:prepack": "turbo run prepack",
"repo:update:conventions": "pnpm meta-updater",
"repo:update:metadata": "node --experimental-strip-types --no-warnings ./repo-metadata/lib/update.ts",
"smoke:setup": "node --disable-warning=ExperimentalWarning --experimental-strip-types ./smoke-tests/node/setup.ts",
"start": "vite",
"test": "node bin/run-tests.mjs",
"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

"test:node": "pnpm turbo test:node",
"test:smoke": "SMOKE_TESTS=true ember test",
"ts": "node --disable-warning=ExperimentalWarning --experimental-strip-types",
"unlink:all": "esyes ./bin/unlink-all.mts"
Expand All @@ -54,8 +55,10 @@
"@eslint-community/eslint-plugin-eslint-comments": "^4.4.1",
"@eslint/config-inspector": "^0.7.1",
"@eslint/js": "9.17.0",
"@glimmer-workspace/benchmark-env": "workspace:*",
"@glimmer-workspace/build-support": "workspace:*",
"@glimmer-workspace/eslint-plugin": "workspace:*",
"@glimmer-workspace/integration-node-tests": "workspace:*",
"@glimmer-workspace/integration-tests": "workspace:*",
"@glimmer-workspace/repo-metadata": "workspace:*",
"@glimmer/env": "0.1.7",
Expand Down Expand Up @@ -126,6 +129,7 @@
"typescript": "^5.7.3",
"typescript-eslint": "^8.19.0",
"vite": "^6.0.10",
"vitest": "^3.0.4",
"zx": "^8.3.0"
},
"changelog": {
Expand Down
1 change: 0 additions & 1 deletion packages/@glimmer-workspace/benchmark-env/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
],
"scripts": {
"prepack": "rollup -c rollup.config.mjs",
"test:lint": "eslint . --quiet",
"test:publint": "publint"
},
"dependencies": {
Expand Down
4 changes: 1 addition & 3 deletions packages/@glimmer-workspace/build/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@
"console"
]
},
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer/local-debug-babel-plugin": "workspace:*",
"@rollup/plugin-commonjs": "^25.0.7",
Expand Down
4 changes: 1 addition & 3 deletions packages/@glimmer-workspace/env/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,5 @@
"version": "0.92.0",
"type": "module",
"exports": "./index.d.ts",
"scripts": {
"test:lint": "eslint . --quiet"
}
"scripts": {}
}
4 changes: 1 addition & 3 deletions packages/@glimmer-workspace/eslint-plugin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
"version": "0.92.0",
"type": "module",
"exports": "./index.js",
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@eslint/eslintrc": "^3.2.0",
"@eslint/js": "^9.18.0",
Expand Down
2 changes: 2 additions & 0 deletions packages/@glimmer-workspace/integration-node-tests/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
packages/
pnpm-lock.yaml
21 changes: 21 additions & 0 deletions packages/@glimmer-workspace/integration-node-tests/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"name": "@glimmer-workspace/integration-node-tests",
"version": "0.92.0",
"type": "module",
"private": true,
"repo-meta": {
"strictness": "loose"
},
"scripts": {
"test:node": "vitest --run"
},
"dependencies": {
"@glimmer/syntax": "workspace:*",
"execa": "^9.5.2",
"prettier": "^3.4.2"
},
"devDependencies": {
"@glimmer-workspace/repo-metadata": "workspace:*",
"vitest": "^3.0.4"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { createRequire } from 'node:module';

import * as prettier from 'prettier';
import { describe, expect, it } from 'vitest';

const require = createRequire(import.meta.url);

/**
* See: https://github.com/glimmerjs/glimmer-vm/issues/1688
*
* Requires the root package.json#pnpm#overrides point at our internal
* copy of @glimmer/syntax, or else prettier brings its own already published
* copy of @glimmer/syntax
*
* NOTE: that this test alone is insufficient to test our built outputs.
* the smoke-tests/* folders are for that purpose.
*/
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.

it(`SMOKE: we've symlinked to the in-repo copy of @glimmer/syntax`, () => {
let workspacePath = require.resolve('@glimmer/syntax');
let prettierPath = require.resolve('prettier');
let prettierGlimmer = require.resolve('@glimmer/syntax', { paths: [prettierPath] });

expect(prettierGlimmer).toBe(workspacePath);
});

it('Underlynig preprocess API works', async () => {
let result = (await import('@glimmer/syntax')).preprocess('<h1></h1>');

expect(result, `It can be await import()'d, and doesn't error`).toBeTruthy();
});

it('Prettier can call preprocess', async () => {
let result = await prettier.format(` <div>\n</div>`, { parser: 'glimmer' });

expect(result).toMatchInlineSnapshot(`
"<div>
</div>"
`);
});
});
4 changes: 1 addition & 3 deletions packages/@glimmer-workspace/integration-tests/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@
"test/**/*"
]
},
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer-workspace/test-utils": "workspace:*",
"@glimmer/compiler": "workspace:*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
"private": true,
"name": "@glimmer-test/integration-tests",
"version": "0.92.0",
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer-workspace/integration-tests": "workspace:*",
"@glimmer-workspace/test-utils": "workspace:*",
Expand Down
4 changes: 1 addition & 3 deletions packages/@glimmer-workspace/test-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
"version": "0.92.0",
"type": "module",
"main": "index.ts",
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer/interfaces": "workspace:*",
"@glimmer/util": "workspace:*"
Expand Down
1 change: 0 additions & 1 deletion packages/@glimmer/compiler/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
],
"scripts": {
"prepack": "rollup -c rollup.config.mjs",
"test:lint": "eslint . --quiet",
"test:publint": "publint"
},
"dependencies": {
Expand Down
4 changes: 1 addition & 3 deletions packages/@glimmer/compiler/test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
"private": true,
"name": "@glimmer-test/compiler",
"version": "0.92.0",
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer/compiler": "workspace:*",
"@glimmer/interfaces": "workspace:*",
Expand Down
4 changes: 1 addition & 3 deletions packages/@glimmer/constants/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
"type": "module",
"main": "index.ts",
"types": "index.ts",
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer/interfaces": "workspace:*"
},
Expand Down
4 changes: 1 addition & 3 deletions packages/@glimmer/constants/test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
"private": true,
"name": "@glimmer-test/constants",
"version": "0.92.0",
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer/env": "0.1.7",
"@glimmer/util": "workspace:*"
Expand Down
4 changes: 2 additions & 2 deletions packages/@glimmer/debug-util/lib/present.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
  }

return list ? list.length > 0 : false;
}

export function ifPresent<T, U, V>(
Expand Down
4 changes: 1 addition & 3 deletions packages/@glimmer/debug-util/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@
"description": "Common utilities used in Glimmer with debug-specific behavior",
"type": "module",
"exports": "./index.ts",
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer/interfaces": "workspace:*"
},
Expand Down
4 changes: 1 addition & 3 deletions packages/@glimmer/debug-util/test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
"private": true,
"name": "@glimmer-test/debug-util",
"version": "0.92.0",
"scripts": {
"test:lint": "eslint . --quiet"
},
"scripts": {},
"dependencies": {
"@glimmer/env": "0.1.7",
"@glimmer/util": "workspace:*"
Expand Down
Loading
Loading