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

Set up Typescript in hybrid mode #800

Merged
merged 11 commits into from
Mar 5, 2025
Merged

Conversation

sergei-maertens
Copy link
Member

@sergei-maertens sergei-maertens commented Mar 3, 2025

Partially closes #445

  • Bumped TS version used. It was present before because it was a dev dependency for some other tooling, but was stuck on an older version.
  • Bumped a bunch of libraries to hopefully pass type checking, however it's impossible given the set of constraints to skipLibCheck requires to be enabled :(
  • Converted some modules/components to TypeScript to prove that the hybrid approach works

Copy link

codecov bot commented Mar 3, 2025

Bundle Report

Changes will decrease total bundle size by 8.08kB (-0.08%) ⬇️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@open-formulieren/sdk-OpenForms-umd 4.78MB -4.04kB (-0.08%) ⬇️
@open-formulieren/sdk-esm 4.76MB -4.05kB (-0.08%) ⬇️

Affected Assets, Files, and Routes:

view changes for bundle: @open-formulieren/sdk-OpenForms-umd

Assets Changed:

Asset Name Size Change Total Size Change (%)
open-*.js -4.04kB 3.5MB -0.12%

Files in open-*.js:

  • ./src/components/AppDisplay.tsx → Total Size: 932 bytes

  • ./src/components/Map/LeafletMap.jsx → Total Size: 7.54kB

  • ./src/components/Body.tsx → Total Size: 282 bytes

  • ./src/Context.js → Total Size: 1.3kB

  • ./src/components/FormDisplay.tsx → Total Size: 579 bytes

  • ./src/headers.ts → Total Size: 610 bytes

view changes for bundle: @open-formulieren/sdk-esm

Assets Changed:

Asset Name Size Change Total Size Change (%)
assets/formio-*.js -1.08kB 2.02MB -0.05%
assets/sdk-*.js -2.95kB 1.01MB -0.29%
assets/LeafletMap-*.js 208 bytes 366.53kB 0.06%
assets/index-*.js -226 bytes 31.07kB -0.72%

Files in assets/sdk-*.js:

  • ./src/headers.ts → Total Size: 610 bytes

  • ./src/components/FormDisplay.tsx → Total Size: 579 bytes

  • ./src/Context.js → Total Size: 1.3kB

  • ./src/components/Body.tsx → Total Size: 282 bytes

  • ./src/components/AppDisplay.tsx → Total Size: 932 bytes

Files in assets/LeafletMap-*.js:

  • ./src/components/Map/LeafletMap.jsx → Total Size: 7.54kB

Copy link

codecov bot commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.15%. Comparing base (4eed457) to head (2cf3ec0).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/components/FormDisplay.tsx 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #800      +/-   ##
==========================================
+ Coverage   83.50%   84.15%   +0.64%     
==========================================
  Files         233      233              
  Lines        4607     4631      +24     
  Branches     1187     1191       +4     
==========================================
+ Hits         3847     3897      +50     
+ Misses        730      704      -26     
  Partials       30       30              
Flag Coverage Δ
storybook 77.63% <88.46%> (+0.81%) ⬆️
vitest 62.18% <65.00%> (-0.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sergei-maertens sergei-maertens force-pushed the chore/445-add-typescript branch 2 times, most recently from c1c7ec1 to 10e03a8 Compare March 4, 2025 09:31
@sergei-maertens sergei-maertens marked this pull request as ready for review March 4, 2025 10:20
Bumped from TS 4.9 to 5.7. TS 5.8 released a couple of days ago
and is not yet properly supported (via peerDependencies) in our
other tooling, so we hold off on that for a while.

@formatjs/cli is updated to match the peerDependency of
typescript so that only a single version is installed in
our dependencies.
The babel presets are no longer requires, since vite/esbuild takes care
of that. The module resolution and formatjs plugins are still necessary
since the tsconfig.json doesn't cover that.

When imports are converted to use the @/ prefix, we can drop the
module resolution plugin.
Now that we're switching to TS, we can fully rely on the Vite toolchain
which no longer requires most of the babel plugins to deal with JSX and
React.
Added some missing type definitions, but not everything is there
yet.
We get some conflicts from leaflet libraries and some @storybook/test shenanigans,
additionally there are some errors from react-use. It's not feasible to address
these right now since we're aiming to get a foot in the door and get
started with typing our own code.

The offending files:

Found 12 errors in 8 files.

Errors  Files
     1  node_modules/@open-formulieren/leaflet-tools/lib/rd.d.ts:8
     1  node_modules/@vitest/expect/dist/chai.d.cts:16
     3  node_modules/react-leaflet-draw/src/index.d.ts:4
     3  node_modules/react-leaflet/lib/LayersControl.d.ts:9
     1  node_modules/react-leaflet/lib/SVGOverlay.d.ts:11
     1  node_modules/react-use/lib/useEnsuredForwardedRef.d.ts:1
     1  node_modules/react-use/lib/usePermission.d.ts:10
     1  node_modules/vitest/node_modules/@vitest/expect/dist/chai.d.cts:16

Hopefully in the future we can re-enable this again.
Turns out we need the babel presets after all for the storybook
processing :( I'm not sure why things work without in the renderer
repo, but we'll figure it out at some point.

This strongly types the Body component with its 'component' prop,
allowing strings of intrinsic elements to be passed. Support for
custom react components is dropped, as it doesn't appear to be
used anywhere so this simplifies things a bit.
* Added a test id for testing-library to the map container so that the
  interaction tests only start running when the container is visible
* Added robustness for interaction elements that may exist in the DOM
  but not be visible yet because map tiles are still loading

waitFor has longer default timeouts than canvas.findBy* queries, and
while the latter can be specified, the element is find before it's
visible and there are still race conditions. For this reason, the
visibility check is decoupled from the lookup expectation.
@sergei-maertens sergei-maertens force-pushed the chore/445-add-typescript branch from 773d4d5 to 2cf3ec0 Compare March 5, 2025 12:46
@sergei-maertens sergei-maertens merged commit a375c8e into main Mar 5, 2025
17 checks passed
@sergei-maertens sergei-maertens deleted the chore/445-add-typescript branch March 5, 2025 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt typescript
1 participant