Move top level modules into utils #2604
Open
Conversation
This file contains 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
rupertbates
reviewed
Dec 18, 2024
| @@ -4,15 +4,5 @@ | |||
| "require": [ | |||
| "tsconfig-paths/register" | |||
| ] | |||
| }, | |||
There was a problem hiding this comment.
Are you sure this is not needed? I think the issue without it was that ts-node couldn't resolve the module paths.
There was a problem hiding this comment.
Pretty sure - these all exist in the top level tsconfig. I might be missing something though.
There was a problem hiding this comment.
This is in fact needed to run pnpm --filter product-catalog generateTypes. Put back in 8f31213.
rupertbates
approved these changes
Dec 18, 2024
There was a problem hiding this comment.
This is great, arguably errors and stage don't really belong in a utils directory. They might be better off in their own specific directory? Don't mind too much about that though.
As I've commented elsewhere, are you sure we don't need those paths in the ts configs?
Supporting files at the top level of the modules dir was a bit of an exception and meant we needed an additional rule in the jest config. It's only one line but it doubled the cognitive load of understanding how the module mappings worked, and it lead to some inconsistency of how modules were imported. Because of the exception, it also meant top level modules tests weren't getting run in CI.
For some reason things break with ts-node when this is removed, like it doesn't properly use the extended tsconfig. This broke: $ pnpm --filter product-catalog generateTypes
tjmw
force-pushed
the
tw/top-level-modules
branch
from
8f31213 to
854c068
Compare
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.
What does this change?
All files at the top level of
moduleshave been moved intoutilsand are now imported as such (e.g.import { getIfDefined } from '@modules/utils/nullAndUndefined'. Supporting files at the top level of the modules dir was a bit of an exception and meant we needed an additional rule in the jest config. It's only one line but it doubled the cognitive load of understanding how the module mappings worked, and it lead to some inconsistency of how modules were imported.Because of the exception, it also meant top level modules tests weren't getting run in CI.
How to test
How can we measure success?
Have we considered potential risks?
Images
Accessibility