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

feat: initialize typescript #60

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

johncardiologs
Copy link
Contributor

@johncardiologs johncardiologs commented May 8, 2024

A first jab at introducing ts/tsx to this repo!
cc @jhash

Copy link
Collaborator

@jho406 jho406 left a comment

Choose a reason for hiding this comment

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

I think this is looking great. Thank you so much.

I want this PR to be sort of a smaller step, so i made comments to keep the babel process while allowing typescript to do checking instead of outputting JS. If you make those smaller changes, i'll merge this. Thanks again!

superglue/lib/components/Nav.tsx Outdated Show resolved Hide resolved
superglue/package.json Outdated Show resolved Hide resolved
superglue/tsconfig.json Outdated Show resolved Hide resolved
@johncardiologs johncardiologs force-pushed the john/feat/add-typescript branch 2 times, most recently from 2223658 to c8a1af5 Compare June 4, 2024 13:58
@johncardiologs
Copy link
Contributor Author

Hi! Sorry, my mind's been completely elsewhere lately and I left this.

Agreed with the approach of keeping babel -- I made the changes you suggested.
I'll look into the failing tests & trying to get this to compile properly when I next have a chance!

@johncardiologs johncardiologs marked this pull request as ready for review June 4, 2024 14:08
Copy link
Collaborator

@jho406 jho406 left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few smaller comments. Don't worry about the typechecking errors. We can work on that on another issue. For this first step, I want this merged in so other contributors can start contributing in typescript.

We just have to make sure that the build output is the same.

superglue/package.json Outdated Show resolved Hide resolved
@jho406
Copy link
Collaborator

jho406 commented Jun 7, 2024

A rebase onto main would fix those errors.

@johncardiologs
Copy link
Contributor Author

johncardiologs commented Jun 8, 2024

To compare the build outputs, I ran the builds and diffd the files. Primarily, I saw one added line in the typescript babel build outputs that was appended to the end of every file:

//# sourceMappingURL=data:application/json;charset=utf-8;base64

followed by a string of characters. It seems this corresponds to babel's source map option.

The only other 'true diff' I saw apart from this was the following, where dist/ contains files built via this PR and maindist/ holds files built using the npm run build:index command from branch main:

❯ diff dist/index.js maindist/index.js
4c4
< exports.superglueReducer = exports.pageReducer = exports.mapDispatchToPropsIncludingVisitAndRemote = exports.getIn = exports.fragmentMiddleware = exports.UPDATE_FRAGMENTS = exports.SAVE_RESPONSE = exports.REMOVE_PAGE = exports.GRAFTING_SUCCESS = exports.GRAFTING_ERROR = exports.COPY_PAGE = exports.BEFORE_VISIT = exports.BEFORE_REMOTE = exports.BEFORE_FETCH = exports.ApplicationBase = void 0;
---
> exports.updateFragments = exports.superglueReducer = exports.pageReducer = exports.mapDispatchToPropsIncludingVisitAndRemote = exports.getIn = exports.fragmentMiddleware = exports.UPDATE_FRAGMENTS = exports.SAVE_RESPONSE = exports.REMOVE_PAGE = exports.GRAFTING_SUCCESS = exports.GRAFTING_ERROR = exports.COPY_PAGE = exports.BEFORE_VISIT = exports.BEFORE_REMOTE = exports.BEFORE_FETCH = exports.ApplicationBase = void 0;
10a11
> exports.updateFragments = _reducers.updateFragments;

I'm not entirely sure what could be causing this difference, but it doesn't seem super harmful to me -- though I'm not entirely familiar yet with superglue!

Let me know if there's anything else to fix up, otherwise I think this may be a decent starting point!

Copy link
Collaborator

@jho406 jho406 left a comment

Choose a reason for hiding this comment

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

I looked through the PR. CI is failing because of eslint. We can ignore that, i'll add another story for that. The output is also slightly different because typescript caught an undefined export updateFragments, which is great!

I had to other comments that i'd like addressed, and i'll merge away! Thanks for working on this.

superglue/package.json Outdated Show resolved Hide resolved
superglue/package.json Outdated Show resolved Hide resolved
@jho406 jho406 merged commit 3677d94 into thoughtbot:main Jun 11, 2024
7 of 8 checks passed
This was referenced Jun 11, 2024
@johncardiologs johncardiologs deleted the john/feat/add-typescript branch June 11, 2024 14:28
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