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

Bun / TSC build structure & comparison #17

Merged
merged 18 commits into from
Oct 16, 2024
Merged

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Oct 9, 2024

Note: includes changes from #16 , review and merge that PR first.

Builds the ds package with both tsc & bun build, roughly following the structure shown here.

This functions as an exploration of the benefits and drawbacks of tsc and bun build.

A comparison of their outcomes and our findings are included in README files introduced by this PR.

@jmuzina
Copy link
Member Author

jmuzina commented Oct 10, 2024

CI fails due to Typescript & Biome disagreeing on whether React imports should use import type React or import React. Biome by default prefers import type React, while Typescript prefers import React.

I am making a change to the biome configuration's jsxRuntime field to fix this. This follows guidance from Biome here. The useImportType rule reads jsxRuntime to decide whether to flag these cases.

We should use "jsxRuntime": "reactClassic" if we use "jsx": "react", as indicated by the above documentation.

@jmuzina jmuzina force-pushed the bun-build branch 2 times, most recently from 31fa805 to ac62f64 Compare October 10, 2024 21:10
@jmuzina jmuzina marked this pull request as ready for review October 10, 2024 21:13
@jmuzina jmuzina changed the title wip: Bun / TSC build structure & comparison Bun / TSC build structure & comparison Oct 10, 2024
@jmuzina jmuzina requested a review from advl October 10, 2024 21:14
Copy link
Contributor

@advl advl left a comment

Choose a reason for hiding this comment

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

It can be merged
Two minor things (for a future PR ?)

  • document the local override of "allowSyntheticDefaultImports": true in ds/tsconfig.json
  • let's try to use the modern react-jsx and make it work with biome ?

@jmuzina
Copy link
Member Author

jmuzina commented Oct 11, 2024

* document the local override of `"allowSyntheticDefaultImports": true` in ds/tsconfig.json

@advl I was able to remove this override without causing any errors - I think that was a holdover from our debugging stages.

* let's try to use the modern `react-jsx` and make it work with biome ?

I was able to safely switch this over to modern react-jsx and undo the JSX runtime override - no obvious errors. I think it can go in this PR given how simple of a change it was.

Let me know if there's something I've missed that needs to be investigated as part of a separate PR.

@jmuzina jmuzina mentioned this pull request Oct 11, 2024
@advl advl merged commit 888b605 into canonical:main Oct 16, 2024
2 checks passed
@jmuzina jmuzina deleted the bun-build branch October 16, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants