-
Notifications
You must be signed in to change notification settings - Fork 180
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
refactor(app): allow "absolute" imports in app js #16288
Merged
Merged
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
This will let us import stuff with "absolute" paths instead of an infinite number of ../. For instance, in app/src/organisms/Devices/ProtocolRun/ProtocolRunHeader/RunHeaderModalContainer/modals/HeaterShakerIsRunningModal/HeaterShakerIsRunningModal.test.ts, instead of `import { renderWithProviders } from '../../../../../../../../__testing-utils__'` you can `import { renderWithProviders } from '/app/__testing-utils__'`. Note that this configuration has to be in multiple different places: - In the app's vite.config.js, which is used for packing - In the root vitest.config.js, which is used for vitest's custom packing for tests - In the app's tsconfig.json and tsconfig-data.json, which is used for tsc parsing inputs - In the specific tsconfig used for eslint so eslint can follow the imports These all have slightly different formats and the ones that use node's path.resolve() have to have a trailing slash added because path.resolve() removes trailing slashes for some reason.
Replace all the imports of (../)+redux with /app/redux.
Agh I'm so mad there's so many of these
Why did this change touch stuff in molecules. I'm so mad
sfoster1
force-pushed
the
add-src-alias
branch
from
September 18, 2024 18:31
39bf51b
to
6526daf
Compare
mjhuff
approved these changes
Sep 18, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweeeeeet. Yeah, I like it a lot. No issues in my IDE navigating around with these aliases, either.
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 PR adds configuration to tsc for typechecking and lint, vite for bundling, and vitest for testing to allow code in
app/
to import things via paths that look absolute from the root of the repo (but are really just a specific binding). Any path in an import or mock statement that starts with/app
will get rewritten to (the appropriate resolved absolute path for)app/src/
.In addition, rewrite any import that was a relative path that used a string of
../
to get all the way to the top of the repo before going down to another top-level directory as such an absolute-path import. There are a lot of these, which is kind of gross. Each commit after the first handles this rewriting for another kind of import target.Note on merging and reviewing
This PR will conflict with basically any change to the app code, so I'm not going to make it always up to date. Rather, please review this as is, including checking it out and building/bundling the app if you want, and when it's approved I'll do all the merge conflicts then.
review requests