refactor: convert remaining JavaScript test files to TypeScript #7199
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndhoule
force-pushed
the
refactor/convert-js-tests-to-ts
branch
11 times, most recently
from
8c10307 to
905f9e6
Compare
ndhoule
force-pushed
the
refactor/convert-js-tests-to-ts
branch
2 times, most recently
from
9d8494e to
d1afe9f
Compare
serhalp
force-pushed
the
refactor/convert-js-tests-to-ts
branch
from
9c643db to
d1afe9f
Compare
ndhoule
force-pushed
the
refactor/convert-js-tests-to-ts
branch
2 times, most recently
from
ea835e2 to
7f451f9
Compare
serhalp
approved these changes
Apr 16, 2025
ndhoule
force-pushed
the
refactor/convert-js-tests-to-ts
branch
2 times, most recently
from
8ad1eaa to
8bf4d2f
Compare
This changeset converts all remaining JavaScript tests to TypeScript. It's sort of a hack job; I used a bulk converter for most of it, then hand-converted the rest. There's varying degrees of "hey, he did a nice job in this file" and "hey, he did a terrible job in this file," but on the whole this is an improvement. I also updated all of the import paths in tests from importing transpiled (`dist/`) code to untranspiled (`src/`) code. Ergonomically this is better--it's confusing to work on tests that aren't necessarily running code that reflects your edits--but really, I was forced into this by spooky conflicts between transpiled types and untranspiled types.
This removes the `assert` calls we were making in `copyTemplateDir`, as well as the tests that exercise this behavior. These asserts probably made more sense in a pre-TypeScript world, but we no longer need them as TypeScript serves the same purpose.
ndhoule
force-pushed
the
refactor/convert-js-tests-to-ts
branch
from
8bf4d2f to
bd6a26e
Compare
ndhoule
added a commit
that referenced
this pull request
Apr 21, 2025
Follow-up to a comment thread in #7199. This changeset adds types for the global configuration file (`$CONFIG_HOME/netlify/config.json`). This is mainly a step forward in documenting the configuration format. The type safety on it isn't ideal; e.g. you can try to read or write properties that don't exist in the config schema. That said, it's an improvement on the read side in the sense that any property you read is typed--even unknown ones default to `undefined`. This comes with two _potential_ breaking changes; I didn't find that they actually broke anything in practice, but they're worth looking out for in review: 1. We now set defaults for some values we did not previously set defaults for, e.g. `users[id: string].auth` defaults to an object. This could only break if a code path assumes e.g. the presence of `auth.github` means the user is authenticated; however, the way the types are structured marks all properties on that object as undefined, so this is a difficult error to make now. 2. We now validate that `users[id: string].id` is set. Failing to set it would make looking up user configuration impossible (we index into this object using the top-level `userId` field), so this prevents a class of mysterious logic errors. Stronger typing caused some issues in tests that mock the global config store; broke the `GlobalConfigStore`'s storage logic out into an adapter and implemented an in-memory adapter for use in tests. There are better ways to solve this problem--tests should be able to mock the global config without resorting to module mocking--but this will do for now. I think the global config store interface could use some work: at this point I think the dot-prop style of setting and getting properties only adds complexity vs something like a Proxy-wrapped object. That said, this is a step forward without committing to a bigger change.
ndhoule
added a commit
that referenced
this pull request
Apr 21, 2025
Follow-up to a comment thread in #7199. This changeset adds types for the global configuration file (`$CONFIG_HOME/netlify/config.json`). This is mainly a step forward in documenting the configuration format. The type safety on it isn't ideal; e.g. you can try to read or write properties that don't exist in the config schema. That said, it's an improvement on the read side in the sense that any property you read is typed--even unknown ones default to `undefined`. This comes with two _potential_ breaking changes; I didn't find that they actually broke anything in practice, but they're worth looking out for in review: 1. We now set defaults for some values we did not previously set defaults for, e.g. `users[id: string].auth` defaults to an object. This could only break if a code path assumes e.g. the presence of `auth.github` means the user is authenticated; however, the way the types are structured marks all properties on that object as undefined, so this is a difficult error to make now. 2. We now validate that `users[id: string].id` is set. Failing to set it would make looking up user configuration impossible (we index into this object using the top-level `userId` field), so this prevents a class of mysterious logic errors. Stronger typing caused some issues in tests that mock the global config store; broke the `GlobalConfigStore`'s storage logic out into an adapter and implemented an in-memory adapter for use in tests. There are better ways to solve this problem--tests should be able to mock the global config without resorting to module mocking--but this will do for now. I think the global config store interface could use some work: at this point I think the dot-prop style of setting and getting properties only adds complexity vs something like a Proxy-wrapped object. That said, this is a step forward without committing to a bigger change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changeset converts all remaining JavaScript tests to TypeScript. It's sort of a hack job; I used a bulk converter for most of it, then hand-converted the rest. There's varying degrees of "hey, he did a nice job in this file" and "hey, he did a terrible job in this file," but on the whole this is an improvement.
I also updated all of the import paths in tests from importing transpiled (
dist/) code to untranspiled (src/) code. Ergonomically this is better--it's confusing to work on tests that aren't necessarily running code that reflects your edits--but really, I was forced into this by spooky conflicts between transpiled types and untranspiled types.