New reflame app 1 #2
Closed
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
lewisl9029
force-pushed
the
new-reflame-app-1
branch
4 times, most recently
from
4d5c28c
to
dfc3a00
Compare
lewisl9029
force-pushed
the
new-reflame-app-1
branch
4 times, most recently
from
15499cb
to
ccba510
Compare
lewisl9029
force-pushed
the
new-reflame-app-1
branch
4 times, most recently
from
1319aeb
to
1c1cc95
Compare
This was referenced Apr 2, 2023
Vadman97
pushed a commit
to highlight/highlight
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
pushed a commit
to highlight/highlight
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 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. 🙂_ There were several places in the frontend codebase where we were importing from `react-router`, a transitive dependency, instead of `react-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 `useParams` from `react-router-dom`, but that does nothing to protect against importing `useParams` from `react-router`. In fact, this was something I [had to fix](38a02f4#diff-5fd6e69a538cbc878adc5b71eb5d9a6d68cd53d6588689284f4ecd9506343681L49) 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](https://rushjs.io/pages/advanced/phantom_deps/) 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](https://reflame.app/) 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 added `react-router` to 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 new [`linked` install strategy](npm/cli#6078), or possibly yarn's [pnpm nodeLinker option](yarnpkg/berry#3338) for a less disruptive migration (though I have no idea if it does what I think it does). ## How did you test this change? <!-- Frontend - Leave a screencast or a screenshot to visually describe the changes. --> I ran `yarn turbo run dev --filter frontend...` locally and poked around the app. ## Are there any deployment considerations? <!-- Backend - Do we need to consider migrations or backfilling data? --> None that I can think of.
Vadman97
pushed a commit
to highlight/highlight
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
to highlight/highlight
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.
lewisl9029
force-pushed
the
new-reflame-app-1
branch
3 times, most recently
from
74f5fbc
to
28580f4
Compare
lewisl9029
force-pushed
the
new-reflame-app-1
branch
from
28580f4
to
dab4bf3
Compare
lewisl9029
force-pushed
the
new-reflame-app-1
branch
2 times, most recently
from
0505c86
to
d7c43ac
Compare
Vadman97
pushed a commit
to highlight/highlight
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
to highlight/highlight
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.
lewisl9029
force-pushed
the
new-reflame-app-1
branch
7 times, most recently
from
ec34198
to
da15787
Compare
lewisl9029
force-pushed
the
new-reflame-app-1
branch
from
da15787
to
9671a3c
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.
Summary
This PR finally enables Reflame for the Highlight web app. 🥳
What does this mean for me?
First you'll need to sign up at https://reflame.app, connect your GitHub account, and ask @Vadman97 for an invite to the highlight organization.
Then, every time you push up a change to the web app (in /frontend) or any of its dependencies, you'll get a Reflame preview link on your commit immediately, usually within 3 seconds, instead of however many minutes it used to take:
In addition, you'll have access to the Reflame VSCode extension for development, which deploys your changes usually within ~500ms of a file save, and then reflects your changes instantly with a full browser refresh in production mode, or with state-preserving React fast refresh in development mode. See development mode in action here:
Kapture.2023-03-21.at.21.38.32.mp4
It's worth noting that Reflame is actually deploying your changes to the internet every time, so you can send these links to yourself to check your changes on another device (even multiple devices simultaneously), or share them with teammates or customers to give them a sneak peak of what you're working on, iterate with their feedback, and have those changes reflected on their browsers in real time (even if they're on the other side of the world)!
How do I even review this? There's like 500 files here?!
All of the changes worth reviewing are contained in the first commit (about 30 files changed), as the second commit only contains files generated using the newly added build scripts.
These scripts and generated files are a temporary stop-gap to support features that don't have first-class support in Reflame yet, specifically:
These are roughly ranked in order of how quickly I think they will get first-class support in Reflame, at which point we'll be able to remove the associated scripts and generated files. Notably, Vanilla Extract is as far down the list as it is because it requires executing user-supplied code as part of its build process, which is going to take quite a bit of work to enable safely in a multitenant system like Reflame (but I do plan on tackling this eventually as I get closer to building features like testing and backend APIs support). Though we may still be able to get rid of the build script sooner than that by building it into the VSCode extension if there's enough demand.
How did you test this change?
A lot of care has gone into making sure your existing local dev workflow works exactly as you're used to (just with a few more scripts running than before), and that the production deployment process remains untouched as well. If you notice any material differences in any of your day to day workflows while trying out this PR, or in the Render preview deploys, please let me know and I'll try to address it ASAP.
I've tried following the docker dev guide here and running
yarn dev:frontend(without thedoppler run --part), and both seem to be working identically as on main as far as I can tell, though for the latter I'm missing a few env vars from doppler so couldn't verify past the login screen, will need your help to make sure everything works as expected there.Are there any deployment considerations?
Definitely would be helpful to get a Render preview for this to poke around in.