Closed adam1658 closed 2 months ago
@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?
@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.
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/
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:
:
src/js/events/Meetup.tsx
:
number is not assignable to 0 | 1 | 2 ....
)src/js/main.ts
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.
@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.
Thanks @neozenith,
I've set up ts-loader to transpileOnly
and fixed the second change in .prettierignore
as well.
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
Fixes #88.
Some typescript errors remain. I see two paths forward now:
Option 1:
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.