Skip to content
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

task(many): Use esbuild in production #15112

Merged
merged 1 commit into from
Apr 12, 2023
Merged

task(many): Use esbuild in production #15112

merged 1 commit into from
Apr 12, 2023

Conversation

dschom
Copy link
Contributor

@dschom dschom commented Mar 29, 2023

Because

  • We want to move forwards with using esbuild in production

This pull request

  • Adds an ADR for using esbuild in production

Issue that this pull request solves

Closes: FXA-5960

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

@dschom dschom requested a review from a team as a code owner March 29, 2023 21:38
Because:
- We want to move forwards with using esbuild in production

This Commit:
- Adds an ADR for using esbuild in production
## Decision Outcome

After a decent amount of discussion, we have decided to switch over to esbuild for our production builds. Other
teams have made the switch and not encountered major issues.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other teams is this referring to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about plural, but I know the Firefox Monitor team switched to esbuild for their big v2 redesign. Amri did a good demo of their new tech stack in a recent 5x8 presentation a couple weeks ago (that I can probably dig up on AirMo if you are interested).

Copy link
Contributor Author

@dschom dschom Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the plural is misleading. I'll qualify this. And yes, I was referring to the Firefox monitor team.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are others, but we definitely seem to be pioneers: https://github.com/search?q=esbuild+user%3Amozilla+path%3A**%2Fpackage.json+lang%3AJSON&type=code

@clouserw might also have more context since I think he was speaking w/ the Monitor team and possibly others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@chenba chenba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm for it since we won't be skipping typechecks. We should also consider using something like https://github.com/rsms/estrella instead of esbuild directly.

@julianpoy
Copy link
Member

julianpoy commented Mar 31, 2023

I'm for it since we won't be skipping typechecks. We should also consider using something like https://github.com/rsms/estrella instead of esbuild directly.

Agreed. TSX would be my suggestion.

@chenba
Copy link
Contributor

chenba commented Mar 31, 2023

Agreed. TSX would be my suggestion.

"tsx is designed to be a drop-in replacement for node". Not quite the same as replacing tsc no?

@dschom
Copy link
Contributor Author

dschom commented Apr 3, 2023

@chenba @julianpoy, I like the notion of using something like TSX or Estrella. I don’t think we need to come to a decision in this ADR, as these feel more like implementation details that can be decided on when speccing out the epic and might require a bit more evaluation anyways, especially when NX is thrown into the mix. They are duly noted though.

It’s worth mentioning that these packages would also compete with esbuild-register. TSX seems to have good traction, Estrella not nearly as much. I’d certainly be in favor of using tsx over esbuild-register as it seems more full featured: https://npmtrends.com/esbuild-register-vs-estrella-vs-tsx

I do have some concerns with Estrella’s project activity. It’s github repo hasn’t seen a commit in almost two years, despite having some seemingly reasonable PRs like this: rsms/estrella#58

@chenba
Copy link
Contributor

chenba commented Apr 4, 2023

I do have some concerns with Estrella’s project activity.

Oh, good point. I didn't notice that.

@pdehaan pdehaan added the ADR label Apr 4, 2023
@julianpoy
Copy link
Member

julianpoy commented Apr 6, 2023

Agreed. TSX would be my suggestion.

"tsx is designed to be a drop-in replacement for node". Not quite the same as replacing tsc no?

Yep, it's not. I'm advocating for us not pre-building our backend TS.

I don’t think we need to come to a decision in this ADR

Agreed!

would also compete with esbuild-register

esbuild-register definitely has it's quirks and isn't really an "officially" supported way of using ESBuild. TSX can be either used standalone, or as a module loader similar to how esbuild-register works currently.

@chenba the nice thing about using something like TSX is any of our stack traces would be shown with TS content and the proper line numbers even if things like interfaces are present in the code which get deleted by TSC.

@chenba
Copy link
Contributor

chenba commented Apr 6, 2023

Agreed. TSX would be my suggestion.

"tsx is designed to be a drop-in replacement for node". Not quite the same as replacing tsc no?

Yep, it's not. I'm advocating for us not pre-building our backend TS.

@julianpoy ok just so I understand fully, you are suggesting that we ship our ts with tsx for production instead of shipping the compiled js in the Docker image liek we do now?

@julianpoy
Copy link
Member

Agreed. TSX would be my suggestion.

"tsx is designed to be a drop-in replacement for node". Not quite the same as replacing tsc no?

Yep, it's not. I'm advocating for us not pre-building our backend TS.

@julianpoy ok just so I understand fully, you are suggesting that we ship our ts with tsx for production instead of shipping the compiled js in the Docker image liek we do now?

@chenba yep!

@dschom dschom merged commit 02548b3 into main Apr 12, 2023
@dschom dschom deleted the FXA-5960 branch April 12, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants