Skip to content

Fix TS support, allowing type-checking in tests#55

Merged
mansona merged 29 commits intomainfrom
nvp/allow-type-checking-in-tests
Jun 19, 2025
Merged

Fix TS support, allowing type-checking in tests#55
mansona merged 29 commits intomainfrom
nvp/allow-type-checking-in-tests

Conversation

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli commented Jun 6, 2025

Resolves: #53

Prior to this PR, tests do not have a tsconfig, and thus no ts/glint things work in tests.

At present, we get a ton of:

[fix:js]   0:0  error  Parsing error: tests/rendering/template-only-test.ts was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProject

This PR:

  • unskips the typescript tests
  • differentiates between dev ts and publish ts (as we do with babel)
  • enables Glint/TS to work in tests

Sort of based off prior explorations here: https://github.com/NullVoxPopuli/nullui/tree/main (note the config directory)

Comment thread files/package.json
Comment thread files/package.json
"@glint/environment-ember-loose": "^1.4.0",
"@glint/environment-ember-template-imports": "^1.4.0",
"@glint/template": "^1.4.0",
"@ember/app-tsconfig": "^1.0.0",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used for the main tsconfig / test app

library tsconfig is now only for publish

Comment thread files/rollup.config.mjs
Comment thread files/tsconfig.json
Comment thread files/.template-lintrc.cjs
Comment thread files/package.json
Comment thread files/package.json Outdated
Comment thread files/package.json Outdated
Comment thread files/rollup.config.mjs Outdated
Comment thread tests/fixtures/typescript/my-addon/src/components/template-only.hbs
Comment thread tests/fixtures/typescript/my-addon/src/components/co-located.js
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review June 7, 2025 17:42
@NullVoxPopuli NullVoxPopuli changed the title Fix type-checking in tests Fix TS support, allowing type-checking in tests Jun 7, 2025
Comment thread files/package.json Outdated
Comment thread files/rollup.config.mjs Outdated
@NullVoxPopuli NullVoxPopuli dismissed mansona’s stale review June 17, 2025 03:53

Unstable switched to real alpha versions

Copy link
Copy Markdown
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking very good. I'm not sure my review should be blocking because most of the comments are quite minor, I'm just starting to review things in the addon-blueprint with a finer tooth comb at the moment 🙈

Comment thread files/.template-lintrc.cjs
Comment thread files/package.json
Comment thread files/rollup.config.mjs
Comment thread files/rollup.config.mjs Outdated
Comment thread files/rollup.config.mjs Outdated
Comment thread files/tsconfig.publish.json
Comment thread tests/smoke-tests/--typescript.test.ts
Comment thread tests/smoke-tests/--typescript.test.ts
Comment on lines +75 to +77
// It's important that we ensure that dist directory is empty for these tests,
// troll-y things can happen with shared dists
await fs.rm(join(addonDir, 'dist'), { recursive: true, force: true });
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to do a beforeEach to set up the component if we're worried about things like this no?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or move this kind of test into a new describe() suite with its own setup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is in a beforeEach + its own describe! 🎉

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no sorry you misunderstand what I said, if we're deleting a folder in the addonDir in a beforeEach then that suggests that we should be creating the addon in the beforeEach to be safe. Does that make sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not. That sounds like work we don't need to do (and currently don't do)

test order isn't guaranteed, and no test should assume prior work has happened (aside from addon creation and fixture copying)

We don't have any cleanep for the addon itself, or the fixture files, so anything that doesn't operate on fixtures is outside the fixture using describe. Any files we don't want, are deleted before each test, because we can't trust they're already gone for other reasons

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel uncomfortable about this 🙈 but we can discuss it in a follow-up PR 👍

Comment thread tests/smoke-tests/--typescript.test.ts Outdated
@NullVoxPopuli NullVoxPopuli force-pushed the nvp/allow-type-checking-in-tests branch from 07abc73 to 7378a8a Compare June 18, 2025 18:19
@NullVoxPopuli NullVoxPopuli requested review from gossi and mansona June 18, 2025 21:22
@aklkv aklkv mentioned this pull request Jun 19, 2025
Copy link
Copy Markdown
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💪

@mansona mansona merged commit 3866a6a into main Jun 19, 2025
13 checks passed
@mansona mansona deleted the nvp/allow-type-checking-in-tests branch June 19, 2025 13:38
@github-actions github-actions Bot mentioned this pull request Jun 19, 2025
@mansona mansona added the enhancement New feature or request label Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

unable to write test file in ts format

3 participants