Conversation
src/js/global.d.ts
Outdated
| export namespace Icons {} | ||
| export namespace Spots {} |
There was a problem hiding this comment.
Got a bit stuck here with an error of the package not being found before build, not sure if this breaks anything or not.
The errors were:
Cannot find module '@stackoverflow/stacks-icons/icons' or its corresponding type declarations.
Cannot find module '@stackoverflow/stacks-icons/spots' or its corresponding type declarations.
There was a problem hiding this comment.
Hmm that's odd. I can't reproduce that locally and the original exports make the icon and spot types available when developing around the codebase.
Is this reported from a regular npm start/npm run build?
There was a problem hiding this comment.
Just want to recommend caution with changing those declaration files. They could cause unexpected type errors for consumers down the line. If changes need to be made here I would recommend to manually test them in Core before merging to see how the TS compiler reacts there. 🙂
There was a problem hiding this comment.
You can replicate by deleting your dist folder to simulate a fresh build.
I've figured it out by using @ts-expect-error, which is what we use in the other files in this folder. Should be good now!
There was a problem hiding this comment.
Ok small update, that works on the CI, but locally it means on the next run it throws an error because it is expecting the error, so @ts-ignore looks the best bet, but I had to push an update to the link rules to allow it with a description.
Alternative would maybe be just be a @ts-nocheck in this file?
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
Thanks for this housekeeping PR @abovedave!
Upgrades all dependencies to latest versions, including typescript.
💯 Thank you!
Adds an .nvmrc file as we've done in other packages.
👍 This seems reasonable enough. I dug around in the stacks-* repos and found that we don't include an .nvmrc file in any of them. Makes me wonder if it's worth adding just for consistency 🤔
Removes the old [concat](https://www.npmjs.com/package/concat) package which was causing ts errors in favour of a native solution.
One fewer dependencies. Nice!
Couple of changes to tsconfig.json to resolve build errors.
I made some notes about this and the changes to package.json scripts. Just a little cleanup.
src/js/global.d.ts
Outdated
| export namespace Icons {} | ||
| export namespace Spots {} |
There was a problem hiding this comment.
Hmm that's odd. I can't reproduce that locally and the original exports make the icon and spot types available when developing around the codebase.
Is this reported from a regular npm start/npm run build?
After first build there is no longer an error produced here, so ignore is probably a better choice
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
This looks reasonable to me. Thanks @abovedave!
3 participants
.nvmrcfile as we've done in other packages.concatpackage which was causing ts errors in favour of a native solution.tsconfig.jsonto resolve build errors.