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

Add Typescript #117

Merged
merged 9 commits into from
May 20, 2024
Merged

Add Typescript #117

merged 9 commits into from
May 20, 2024

Conversation

adam1658
Copy link
Contributor

@adam1658 adam1658 commented May 16, 2024

Fixes #88.

  • switch webpack to ts-loader instead of babel-loader (remove babel) (following https://webpack.js.org/guides/typescript/)
  • rename all js src files to ts/tsx (except codegen script for events data). Add types
  • update the fetch-events script (still js) to generate ts file, not js file.

Some typescript errors remain. I see two paths forward now:

Option 1:

  • I fix the TS errors I've introduced, including some bugs that this TS has uncovered.
  • Add github actions to prevent merge while TS errors exist.

Option 2:

I leave the TS errors unfixed, providing low hanging fruit for other contributors. I'd open an issue for 'fixing typescript errors and adding typechecking to github actions'. I'd also need to configure webpack to allow build while typescript errors still exist.

@adam1658 adam1658 force-pushed the typescript branch 2 times, most recently from 7d09c7f to ca94285 Compare May 16, 2024 02:06
@adam1658
Copy link
Contributor Author

@neozenith I started this PR in the dev sprint yesterday at /NEW. Are you the best person to talk to about it? I'm not sure what's the best way to finish it off - fix all the typescript errors, or leave it with typescript errors (ensuring that they're ignored so it still builds successfully) - so that'll be a good task for another collaborator to dive into?

@neozenith
Copy link
Member

@adam1658 thanks for putting this together. Just on my phone at the moment. I should get onto my laptop and try the CodeSpaces to check this still builds.

At the moment I’d lean towards Option 1 that you mentioned.

But… I’d like to better understand what the outstanding errors are.

Would the errors block it being able to be built and functionally run the website?

If they wouldn’t functionally block the website being built then Option 2 is viable and we could land this to keep the scope smaller.

@neozenith neozenith self-assigned this May 16, 2024
Copy link
Member

@JayRovacsek JayRovacsek left a comment

Choose a reason for hiding this comment

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

Firstly; thanks very much for your contribution!

A few nits in comments ❤️

src/js/communities/types.ts Outdated Show resolved Hide resolved
src/js/events/types.ts Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
@neozenith
Copy link
Member

Just checked this out in code spaces... Definitely want to pursue Option 1 as the type errors are breaking the build. There is no new build output emitted whilst it is broken.

If you could also change .prettierignore to this contents while you're there:

src/js/events/events-data.ts
src/js/communities/community-data.ts
dist/

@adam1658
Copy link
Contributor Author

Hi @neozenith,

To get a successful build you'll need to modify webpack.config.js to set ts-loader to transpile only. Update it as follows:

-        use: ["ts-loader"],
+        use: [
+          {
+            loader: "ts-loader",
+            options: {
+              transpileOnly: true,
+            },
+          },
+        ],

The typescript errors can be summarised as follows:

  • src/js/communities/Communities.tsx::
    • Preact component returns array - fine with preact but a problem for the types.
    • DOM element may be null.
  • src/js/events/Meetup.tsx:
    • DOM element may be null
    • null handling of properties in event data (nullable properties defined as nullable if graphql schema doesn't say they're required)
    • Referencing property in event data that does not exist
    • indexing into object with numeric keys (map of day number to day name) with number (number is not assignable to 0 | 1 | 2 ....)
  • src/js/main.ts
    • function is not a constructor.

I'm very happy to fix these - I just thought it would be a great opportunity for a newer collaborator to fix some issues that tsc helpfully points out.

I'll have prettier ignore /dist/ as requested.

@neozenith
Copy link
Member

@adam1658 yeah ok.

If you could change the tsconfig that’d be great.

Also the was a second sneaky change in the .prettierignore I’ll let you copy paste my snippet and see in the diff the change I mean. Took me a few minutes to figure out why it wasn’t working.

@adam1658
Copy link
Contributor Author

Thanks @neozenith,

I've set up ts-loader to transpileOnly and fixed the second change in .prettierignore as well.

@neozenith
Copy link
Member

neozenith commented May 18, 2024

Awesome I’ll review and merge this later tonight. (Or if someone else with review permissions wants to they can).

We can take it out of Draft at this point too I guess.

and I capture the scope of the open typescript issues in #119

@neozenith neozenith marked this pull request as ready for review May 18, 2024 23:56
@neozenith neozenith changed the title WIP: Add Typescript Add Typescript May 19, 2024
Copy link
Member

@neozenith neozenith left a comment

Choose a reason for hiding this comment

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

Reviewed. Looks good. Just needs to resolve conflicts with main. So merge main into this branch but accept your changes and ignore conflicts main induces.

I would have done this myself but I don't have permissions to edit back to your fork/branch.

@neozenith neozenith merged commit 47a7609 into newwwie:main May 20, 2024
JayRovacsek pushed a commit that referenced this pull request Dec 1, 2024
* WIP: Add Typescript

* Fix webpace resolve extensions

* Lexicographically sort type attributes

* Prettier: ignore dist/

* Fix .prettierignore community-data.ts

* Set ts-loader to traspileOnly to allow builds with TS errors

* Update readme with message about typescript

* Run fetchEvents
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Typescript
3 participants