-
-
Notifications
You must be signed in to change notification settings - Fork 64
feat(remix): Add wizard for Remix. #387
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
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Add wizard for Remix ([#387](https://github.com/getsentry/sentry-wizard/pull/387)) If none of the above apply, you can opt out of this check by adding |
30b6fad
to
09bc1fb
Compare
09bc1fb
to
5dfb6fb
Compare
787b7f0
to
900c8c2
Compare
900c8c2
to
edf5a51
Compare
src/remix/sdk-setup.ts
Outdated
* Copied from sveltekit wizard | ||
* We want to insert the init call on top of the file but after all import statements | ||
*/ | ||
function getInitCallInsertionIndex(originalHooksModAST: Program): number { |
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.
If this is from the sveltekit wizard, @Lms24 do you think it's appropriate if we extract into a util?
src/remix/sdk-setup.ts
Outdated
return remixConfigModule?.default || {}; | ||
} catch (e: unknown) { | ||
clack.log.error( | ||
`Couldn't load ${REMIX_CONFIG_FILE}. Please make sure, you're running this wizard with Node 14 or newer`, |
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.
The wizard should we Node 14+ - is the another error message we can write?
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.
Just removed it, but can't think of any other possible common issues to point out here.
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.
Round 2 - thanks for sticking with this!
src/remix/sdk-setup.ts
Outdated
const copiedBodyNodes = [...originalHooksModAST.body]; | ||
const lastImportDeclaration = copiedBodyNodes | ||
.reverse() | ||
.find((node) => node.type === 'ImportDeclaration'); | ||
|
||
const initCallInsertionIndex = lastImportDeclaration | ||
? originalHooksModAST.body.indexOf(lastImportDeclaration) + 1 | ||
: 0; | ||
return initCallInsertionIndex; |
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.
can we just run this in a for loop? More efficient than cloning, and iterating through twice.
for (let x = originalHooksModAST.body.length - 1; x >= 0; x--) {
if (originalHooksModAST.body[x].type === 'ImportDeclaration') {
return x + 1;
}
}
return 0;
src/remix/sdk-setup.ts
Outdated
dsn: string, | ||
originalHooksMod: ProxifiedModule<any>, | ||
) { | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment |
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.
can we disable this lint rule for the entire file?
src/remix/sdk-setup.ts
Outdated
originalHooksModAST.body.splice( | ||
initCallInsertionIndex, | ||
0, | ||
// @ts-ignore - string works here because the AST is proxified by magicast |
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.
let's make all these @ts-expect-error
src/remix/sdk-setup.ts
Outdated
|
||
if (hasSentryContent(originalEntryClient, originalEntryClientMod.$code)) { | ||
// Bail out | ||
clack.log.warn( |
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.
Since hasSentryContent already logs out, do we need to log out again here?
src/remix/sdk-setup.ts
Outdated
} else { | ||
if ( |
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.
Can this become else if
?
src/remix/sdk-setup.ts
Outdated
@@ -0,0 +1,509 @@ | |||
import type { ExportNamedDeclaration, Program } from '@babel/types'; |
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.
Could we split up some of the utils in this file into their own files? Specifically I think anything using recast should live in their own file.
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.
Moved under codemods
folder 👍
Co-authored-by: Abhijeet Prasad <[email protected]>
9bd4715
to
2ce03a2
Compare
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.
lgtm! lets wait till @Lms24 double checks though
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.
Looks great, thanks for figuring all of this out Onur! Really like the usage of recast whenever magicast doesn't work.
I still had a few comments but I think we're almost ready to get this in.
I also have a follow-up request but this should be done in a separate PR: SvelteKit and Next wizards automatically create a sample page that throws some client/server errors. Can we do something like this in the Remix wizard, too? If you think this is doable, I'll create an issue to track it.
await instrumentRootRoute(isV2, isTS); | ||
} catch (e) { | ||
clack.log.warn(`Could not instrument root route. | ||
Please do it manually using instructions from https://docs.sentry.io/platforms/javascript/guides/remix/`); |
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.
I think for now this is fine. Once we put the wizard instructions into docs, I think it's likely that we'll refactor the remix docs so that the manual setup will be in its own page (similarly to Next and Svelte). We just have to adjust the links then. I added a task in the follow-up issue (getsentry/sentry#54615)
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.
Damn, almost missed that you also already created a sourcemaps wizard flow. Thanks a lot!
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.
I just ran the wizard in a small remix app and found some more things (thanks for sticking with us here!):
-
Can we log out a
clack.log.success
for each file we touched/inserted code? I think this increases transparency for users so that they are informed what the wizard is doing (especially for those who ran it without being in a version controlled environment).
Co-authored-by: Lukas Stracke <[email protected]>
65fab1e
to
f4be4c9
Compare
@Lms24, @AbhiPrasad -- thanks for the reviews! Updated the PR. |
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.
Thanks for applying our feedback, Onur! Had one last comment then we can merge!
In preparation to merging, would you mind adding a note to CHANGELOG.md
? In this repo, changelogs are semi-automatically generated. For each user-facing PR. we add an entry to CHANGELOG.md under (##Unreleased) and our release process will add it to the version entry for the version to be released. I think for this change, a small usage example would be nice. See the entry for the SvelteKit wizard.
src/sourcemaps/tools/remix.ts
Outdated
${chalk.dim(` | ||
... | ||
"scripts": { | ||
"build": "remix build --sourcemap && sentry-upload-sourcemaps" | ||
} | ||
...`)} |
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.
Let's please directly use console.log
here and adjust the colors to make the build
line stand out. For reference, feel free to take a look at the other tools
flows of the sourcemap wizard (for example, tsc
).
Reason: By directly using console.log, users can more easily copy/paste the code as no clack UI border characters are in the line.
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.
Perfect, thanks Onur!
Resolves: #364
Resolves: getsentry/sentry-javascript#8556
Resolves: getsentry/sentry-javascript#5486
Adds a wizard for auto-instrumenting Remix applications / generating and uploading sourcemaps.
.sentryclirc
build
script inpackage.json
to generate and upload sourcemaps.root.tsx
/root.jsx
for Remix versions 1 and 2.entry.client
andentry.server
for Remix versions 1 and 2.Used
recast
for parts thatmagicast
does not support yet.