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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: switch build to TS + all tests 馃帀 #1038

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

golota60
Copy link
Collaborator

@golota60 golota60 commented Jan 29, 2023

above and beyonce

things done:

  • switched out build to use ACTUAL TS TYPES(this adjusts types so next release will be slightly breaking. would appreciate if you checked the branch out and compared old&new build artifacts(i did and it looks 馃憣 but would appreciate a second opinion to be sure)
  • remove old types
  • converted all tests to TS(should prolly also switch out enzyme)
  • bootstraps TS example from types/ and adds it to example dir

@golota60 golota60 requested a review from jquense January 29, 2023 10:08
@jquense
Copy link
Member

jquense commented Jan 29, 2023

Damn man this awesome.

@golota60
Copy link
Collaborator Author

golota60 commented Feb 7, 2023

i've taken a better glance at build differences - all seem to be related to code-splitting, here they are(left is old right is new)

  1. package.json slight differences(non-issue imo)
    image
  2. minifying differences(non-issue imo)
    image
  3. TS declaration file changes - declaration file changes. they just differ cause they're not static declaration files, but are generated by TS. We didn't remove any types, they are just not so neatly packed together as they were previously, they are just a little bit more over the place(will try to fix that in the coming days by moving stuff from typeUtils.ts to files themselves, instead of a one big file)
    image
  4. server.tsx missing(this just seems random, think it happened cause stuff wasn't explicitly exported - fixed that in newest commit).

So yeah, i think merging this probably won't introduce anything wrong, but since it's a whole switch from JS->TS i expect that some slight issues might come up
i've used meld to compare the changes.

Overall I'd say this is ready to be merged(like I said, I expect issues to be small to nonexistent with this), although I'd understand if we want to turn this into a feature branch or something. I'd say we should face it head on and merge this and then deal with any issues while releasing 馃槃 but that's just me

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.

None yet

2 participants