-
Notifications
You must be signed in to change notification settings - Fork 50
feat: Add individual bundler packages #42
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
Lms24
left a comment
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 some minor optional nits but totally happy to revisit this later)
| { | ||
| "presets": ["@babel/env", "@babel/typescript"] | ||
| } |
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.
Totally optional:
I think the values here are the same for all projects, right? Can we extract this file into one package?
| module.exports = { | ||
| testEnvironment: "node", | ||
| transform: { | ||
| "^.+\\.(t|j)sx?$": ["@swc/jest"], | ||
| }, | ||
| }; |
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.
Totally optional:
Any change, we can also extract this to a central package?
We might wanna add a "dev-utils" package (I know this sounds oddly familiar....)
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 we notice that having stuff duplicated becomes a problem we can extract this. I usually don't like drying things too early. Same goes for other comments.
| @@ -0,0 +1,35 @@ | |||
| import resolve from "@rollup/plugin-node-resolve"; | |||
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.
Totally optional:
Might also be a candidate for a centralized config?
| @@ -0,0 +1 @@ | |||
| dist | |||
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.
Optional:
Hmmm WDYT about adding something like packages/*/dist into the root .gitignore instead?
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.
More risky/opaque as to why something is "gitignored". I'll leave it for now.
Remove our `SentryFacade` abstraction from the project. As we got rid of Sentry CLI (#34) there is no need anymore to have this additional layer and we can just call the facade's functions directly. Additionally, the PR introduces the `SentryContext` type which should hold "sentry internal" values/data, like the hub and the parent span. We might wanna add the logger here later (cc @vladanpaunovic). This replaces the arguments we pass down to the release pipeline functions so we can add more stuff w/o having to add yet another param.
No description provided.