-
Notifications
You must be signed in to change notification settings - Fork 473
Import from react-router-dom instead of react-router #4848
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
Merged
Vadman97
merged 2 commits into
highlight:main
from
lewisl9029:import-from-react-router-dom
Apr 7, 2023
Merged
Import from react-router-dom instead of react-router #4848
Vadman97
merged 2 commits into
highlight:main
from
lewisl9029:import-from-react-router-dom
Apr 7, 2023
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
This was referenced Apr 6, 2023
Vadman97
pushed a commit
that referenced
this pull request
Apr 6, 2023
## Summary <!-- Ideally, there is an attached GitHub issue that will describe the "why". If relevant, use this section to call out any additional information you'd like to _highlight_ to the reviewer. --> _This is part of a [series](#4813) [of](#4848) PRs being spun off from [my WIP branch](lewisl9029#2) to get the Highlight web app ready for [Reflame](https://reflame.app/). Hopefully this makes things a bit easier to review, test, and merge. 🙂_ I ran into this specific [esbuild issue](evanw/esbuild#1836) when preping the dependencies bundle for the [Highlight app](https://highlight-test-lewisl.reflame.dev/~r/start-preview/?mode=production&userId=01FQZZ7XJFDA799Z1Z9DRCFXWA&variantId=01GSY56NZ2GP8KAA4Y26A1RT6E&variantName=git%7Enew-reflame-app-1&resourceIdHtml=YvIo8Nr9gSIBHlECxeibDghiBAU) in [Reflame](https://reflame.app/). And my workaround was to point to specific entry points that we were actually using instead of from the root entry point. Turns out, this also had a _dramatic_ impact on bundle size for the production Vite build: Before: ``` build/assets/index2.js 1,463.51 kB │ gzip: 364.69 kB │ map: 4,645.33 kB build/assets/index.js 7,624.60 kB │ gzip: 2,187.98 kB │ map: 25,251.61 kB ``` After: ``` build/assets/index2.js 1,463.51 kB │ gzip: 364.78 kB │ map: 4,644.97 kB build/assets/index.js 6,753.71 kB │ gzip: 1,903.80 kB │ map: 23,556.41 kB ``` Saves almost 300kb gzipped! It looks like the default entry point of `react-syntax-highlighter` contains import statements for basically all variantions (sync vs async), all languages and all themes that apparently weren't being tree shaken out properly. ## How did you test this change? <!-- Frontend - Leave a screencast or a screenshot to visually describe the changes. --> I ran the app using `yarn turbo run dev --filter frontend...` but still haven't figured out how to get past the signin screen there. However, I did apply the same change in Reflame and was able to verify syntax highlighting still looked fine on the setup pages:  ## Are there any deployment considerations? <!-- Backend - Do we need to consider migrations or backfilling data? --> Probably worth poking around in a Render preview as well just to be safe.
Vadman97
approved these changes
Apr 7, 2023
Comment on lines
+110
to
+113
| { | ||
| "name": "react-router", | ||
| "message": "Please use `react-router-dom` instead." | ||
| }, |
Member
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.
great to prevent this going forward with an eslint rule!
Vadman97
pushed a commit
that referenced
this pull request
Apr 7, 2023
## Summary <!-- Ideally, there is an attached GitHub issue that will describe the "why". If relevant, use this section to call out any additional information you'd like to _highlight_ to the reviewer. --> _This is part of a [series](#4813) [of](#4848) [PRs](#4849) [being](#4851) spun off from [my WIP branch](lewisl9029#2) to get the Highlight web app ready for [Reflame](https://reflame.app/). Hopefully this makes things a bit easier to review, test, and merge. 🙂_ We were on v8 of the firebase SDK before, which had a whole bunch of issues when I tried to get it running on the [Highlight app](https://highlight-test-lewisl.reflame.dev/~r/start-preview/?mode=production&userId=01FQZZ7XJFDA799Z1Z9DRCFXWA&variantId=01GSY56NZ2GP8KAA4Y26A1RT6E&variantName=git%7Enew-reflame-app-1&resourceIdHtml=YvIo8Nr9gSIBHlECxeibDghiBAU) on [Reflame](https://reflame.app/) due to its esoteric module structure. So I looked into what it would take to get it upgraded to v9, and apparently they made things pretty straightforward with `firebase/compat/*` entry points we could use to upgrade without having to change any of our downstream code, just the imports. That's all this PR does. Bumps firebase to the latest v9, and replaces all of our imports with `firebase/compat/*`. It also came with a slight ~15KB bundle size savings, likely due to the more tree-shakable module structure: Before: ``` build/assets/index2.js 1,463.51 kB │ gzip: 364.69 kB │ map: 4,645.33 kB build/assets/index.js 7,624.60 kB │ gzip: 2,187.98 kB │ map: 25,251.61 kB ``` After: ``` build/assets/index2.js 1,463.51 kB │ gzip: 364.69 kB │ map: 4,645.33 kB build/assets/index.js 7,671.42 kB │ gzip: 2,171.71 kB │ map: 25,506.68 kB ``` ## How did you test this change? <!-- Frontend - Leave a screencast or a screenshot to visually describe the changes. --> I ran the app using yarn turbo run dev --filter frontend... but still haven't figured out how to get past the signin screen there. But I have been poking around on the [Reflame preview](https://highlight-test-lewisl.reflame.dev/~r/start-preview/?mode=production&userId=01FQZZ7XJFDA799Z1Z9DRCFXWA&variantId=01GSY56NZ2GP8KAA4Y26A1RT6E&variantName=git%7Enew-reflame-app-1&resourceIdHtml=YvIo8Nr9gSIBHlECxeibDghiBAU) of the app with this version of firebase for quite a while now, and haven't noticed any related issues. ## Are there any deployment considerations? <!-- Backend - Do we need to consider migrations or backfilling data? --> Probably worth deploying to a Render preview and poking around there before merging.
Vadman97
pushed a commit
that referenced
this pull request
Apr 10, 2023
## Summary <!-- Ideally, there is an attached GitHub issue that will describe the "why". If relevant, use this section to call out any additional information you'd like to _highlight_ to the reviewer. --> _This is part of a [series](#4813) [of](#4848) [PRs](#4849) being spun off from [my WIP branch](lewisl9029#2) to get the Highlight web app ready for [Reflame](https://reflame.app/). Hopefully this makes things a bit easier to review, test, and merge. 🙂_ The sdk/firstload package currently imports from `@highlight-run/client` using relative paths that point outside the package root. I read your [blog post on this](https://www.highlight.io/blog/publishing-private-pnpm-monorepo) and was able to get a good understanding of the motivations, but it was breaking the app in Reflame (where individual libraries are much more tightly isolated at the boundary, and relative imports outside the package root would basically just point into the aether) so I had to find a workaround. 😅 I think I found a reasonable one here, where we use the [`modulePaths` option](https://github.com/rollup/plugins/tree/master/packages/node-resolve#modulepaths) in `@rollup/plugin-node-resolve` to make `@highlight-run/client` resolvable as an internal module, so it gets included in the bundle without the need to specify it as a dependency in package.json. The resulting builds ended up having identical hashes as before the change, so this did manage to preserve the same output as before. Wdyt? ## How did you test this change? I added `entryFileNames: '[name]-[hash].js'` to output options and ran the build in this branch and main. Both ended up with identical hashes, which means the build output was not affected at all by this change. 😄 <!-- Frontend - Leave a screencast or a screenshot to visually describe the changes. --> ## Are there any deployment considerations? <!-- Backend - Do we need to consider migrations or backfilling data? --> Since the build output is identical I think we're good, but it could be a good idea to cut a new release just in case to confirm this doesn't cause any unforeseen issues in the wild.
This was referenced Apr 10, 2023
Vadman97
pushed a commit
that referenced
this pull request
Apr 10, 2023
## Summary <!-- Ideally, there is an attached GitHub issue that will describe the "why". If relevant, use this section to call out any additional information you'd like to _highlight_ to the reviewer. --> _This is part of a [series](#4813) [of](#4848) [PRs](#4849) [being](#4851) [spun](#4852) off from [my WIP branch](lewisl9029#2) to get the Highlight web app ready for [Reflame](https://reflame.app/). Hopefully this makes things a bit easier to review, test, and merge. 🙂_ Previously we had a bunch of places where we imported `/src/style/common.css` using the identifier `style/common.css`. `style/*` was not setup as an alias in tsconfig.json, but this seem to have worked anyways (I think it might be a quirk of the https://github.com/aleclarson/vite-tsconfig-paths plugin). I don't think it's a good idea to rely on this behavior over the long term since there's nothing distinguishing it from npm package imports. We could have worked around this by adding a `@styles` identifier like we have for most other top level folders, but in this PR I proposed what I believe is a more flexible option of simply exposing the src directory as a `@/*` alias. This has several benefits over manually setting up aliases for top level folders separately: - We can use it to import files on the top level as well (in my WIP PR I added a env.ts module that didn't make a lot of sense anywhere else) - Imports from `@/*` matches the filesystem directory structure exactly, so there's never any ambiguity to where an imported file actually lives on disk (in our current setup, we have 2 aliases that don't map 1:1 to top level folders `@icons/*` and `@graph/*`) - All else being equal, more path remapping rules will result in worse performance for module resolution compared to fewer (though I haven't ran the numbers to quantify this yet) That said, this PR just introduces the new alias and uses it for `@/style/common.css`, and doesn't change any other existing imports, so none of these benefits are actually realized here. Though if the team is interested in moving forward with this, I'd be happy to open up a follow up PR to update imports throughout the rest of the codebase as well, and try running a few benchmarks to see if it actually moves the needle on performance. Alternatively, let me know if y'all prefer to keep the current approach using manual top level path remappings instead. I'd be happy to switch over to a `@styles/*` import here as well. ## How did you test this change? <!-- Frontend - Leave a screencast or a screenshot to visually describe the changes. --> I ran the app using `yarn turbo run dev --filter frontend...`. ## Are there any deployment considerations? <!-- Backend - Do we need to consider migrations or backfilling data? --> None that I'm aware of.
Vadman97
pushed a commit
that referenced
this pull request
Apr 10, 2023
## Summary <!-- Ideally, there is an attached GitHub issue that will describe the "why". If relevant, use this section to call out any additional information you'd like to _highlight_ to the reviewer. --> _This is part of a [series](#4813) [of](#4848) [PRs](#4849) [being](#4851) [spun](#4852) [off](#4907) from [my WIP branch](lewisl9029#2) to get the Highlight web app ready for [Reflame](https://reflame.app/). Hopefully this makes things a bit easier to review, test, and merge. 🙂_ We had both a `CommentTextBody.module.css` and a `CommontTextBody.module.scss` previously. The scss module seems to be for regular component styling, while the css module seemed to be mostly meant to contain classes for the `@highlight-run/react-mentions` classNames prop integration. I ended up renaming `CommentTextBody.module.css` to `mentions.module.scss` to better reflect its purpose and distinguish it from the main styling module, and so there wouldn't be any name conflicts when generating corresponding .js modules for the Reflame integration. There also seemed to be 2 classes in the css module that didn't have anything to do with mentions, so I moved those out as well. ## How did you test this change? <!-- Frontend - Leave a screencast or a screenshot to visually describe the changes. --> Tested this out in the Reflame preview:  ## Are there any deployment considerations? <!-- Backend - Do we need to consider migrations or backfilling data? --> We do probably want to verify in Render as well to make sure Vite has the same exports output for CSS and SCSS modules.
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.
Summary
This is part of a series of PRs being spun off from my WIP branch to get the Highlight web app ready for Reflame. Hopefully this makes things a bit easier to review, test, and merge. 🙂
There were several places in the frontend codebase where we were importing from
react-router, a transitive dependency, instead ofreact-router-dom, a direct dependency.This is often referred to as a phantom dependency, and can cause a number of issues, both in theory and practice:
We have a rule against importing
useParamsfromreact-router-dom, but that does nothing to protect against importinguseParamsfromreact-router. In fact, this was something I had to fix as part of this PR.Since the versions of transitive deps are not specified explicitly, they can easily change under our feet without us noticing and potentially cause us to import a different version than we were expecting, or can even inexplicably disappear altogether when seemingly unrelated deps change. The potential for spooky actions at a distance is greatly exacerbated in a large monorepo like we have here. The Rush.js folks did a great writeup on the perils of phathom dependencies, so I won't rehash all the details.
It's incompatible with stricter package management schemes like pnpm (and the one used by Reflame itself, which was admittedly my primary motivation for this PR 😅) that don't expose non-direct dependencies to the resolution algorithm to begin with, specifically to combat the phantom dependency problem.
All I did to fix this was to search & replace all references of
'react-router'to'react-router-dom'. And to prevent this specific issue from happening again, I addedreact-routerto our existing list of restricted imports in eslint. For a more thorough defense against phantom deps, we'll need to switch to something like pnpm, npm's newlinkedinstall strategy, or possibly yarn's pnpm nodeLinker option for a less disruptive migration (though I have no idea if it does what I think it does).How did you test this change?
I ran
yarn turbo run dev --filter frontend...locally and poked around the app.Are there any deployment considerations?
None that I can think of.