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

Adopt typescript #445

Open
3 of 5 tasks
Tracked by #724
sergei-maertens opened this issue Jun 9, 2023 · 0 comments · Fixed by #800, #802 or #804
Open
3 of 5 tasks
Tracked by #724

Adopt typescript #445

sergei-maertens opened this issue Jun 9, 2023 · 0 comments · Fixed by #800, #802 or #804
Assignees

Comments

@sergei-maertens
Copy link
Member

sergei-maertens commented Jun 9, 2023

Check if we can start gradual typescript adoption, normally CRA should support it out of the box?

Steps:

  • Use @open-formulieren/formio-renderer where possible to reduce the amount of code that needs converting
  • Add typescript dependency and config file(s)
  • Upgrade dependencies
  • Convert low-hanging fruit
  • Convert entrypoint and publish type declaration files
@sergei-maertens sergei-maertens self-assigned this Jun 9, 2023
@sergei-maertens sergei-maertens moved this from Todo to In Progress in Development Feb 27, 2025
sergei-maertens added a commit that referenced this issue Mar 3, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 3, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 3, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 3, 2025
Added some missing type definitions, but not everything is there
yet.
sergei-maertens added a commit that referenced this issue Mar 3, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 3, 2025
Added some missing type definitions, but not everything is there
yet.
sergei-maertens added a commit that referenced this issue Mar 3, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 3, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 4, 2025
* 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 added a commit that referenced this issue Mar 5, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 5, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 5, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 5, 2025
Added some missing type definitions, but not everything is there
yet.
sergei-maertens added a commit that referenced this issue Mar 5, 2025
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.
sergei-maertens added a commit that referenced this issue Mar 6, 2025
Doing a click + ArrowDown on a react select seems to have a race
condition where sometimes the *next* option gets 'focus', resulting in
it being higlighted with a different color, which in turn trips the
Chromatic snapshots as it's toggling between which option is focused.

Replacing this with an explicit focus + ArrowDown causes the menu to
open and seems to avoid the race condition.

The click used to be required in older versions of testing library and
react-select to properly focus the input/select when the browser window
itself doesn't have focus, e.g. when live-reloading saved code in SB
which would lead to a frustrating DX.
sergei-maertens added a commit that referenced this issue Mar 6, 2025
Adapted the few places where we directly import the mixin
for certain components, which has been moved into the package
src and is no longer in the dist/css directory.

Other imports have been updated to refer to the package itself,
as that resolves to the CSS file inside, and we can now use
the @use rule in a bunch of places rather than sticking with
the deprecated @import.
sergei-maertens added a commit that referenced this issue Mar 6, 2025
Contains fix for the breaking alert component changes.
sergei-maertens added a commit that referenced this issue Mar 6, 2025
Contains fix for the breaking alert component changes.
sergei-maertens added a commit that referenced this issue Mar 6, 2025
The real test to see if we have broken CSS 😬
sergei-maertens added a commit that referenced this issue Mar 6, 2025
The latter is typescript compatible.
sergei-maertens added a commit that referenced this issue Mar 6, 2025
And immediately there is a type issue where strings are passed down
to FormattedNumber, which is invalid.
sergei-maertens added a commit that referenced this issue Mar 6, 2025
Explicit @/ imports are preferred as they deliberately signal that
it's code from our own root/src dir. This will eventually allow us to
clean up the rest of the import sorting config.
sergei-maertens added a commit that referenced this issue Mar 6, 2025
Type checker reported bugs:

* subtracting dates from each other (sort-of false positive...)
* the way we handle params/headers in API calls didn't account for
  null
* some parseInt call had the radix _outside_ of the function call
sergei-maertens added a commit that referenced this issue Mar 6, 2025
sergei-maertens added a commit that referenced this issue Mar 10, 2025
And immediately there is a type issue where strings are passed down
to FormattedNumber, which is invalid.
sergei-maertens added a commit that referenced this issue Mar 10, 2025
Explicit @/ imports are preferred as they deliberately signal that
it's code from our own root/src dir. This will eventually allow us to
clean up the rest of the import sorting config.
sergei-maertens added a commit that referenced this issue Mar 10, 2025
Type checker reported bugs:

* subtracting dates from each other (sort-of false positive...)
* the way we handle params/headers in API calls didn't account for
  null
* some parseInt call had the radix _outside_ of the function call
sergei-maertens added a commit that referenced this issue Mar 10, 2025
Dropped the value prop in favour of always passing children.
sergei-maertens added a commit that referenced this issue Mar 11, 2025
Dropped the value prop in favour of always passing children.
@github-project-automation github-project-automation bot moved this from In Progress to Done in Development Mar 11, 2025
@github-project-automation github-project-automation bot moved this from Done to In Progress in Development Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
1 participant