-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Convert project to TypeScript #52
Conversation
Two more notes on this:
|
Wow, this is really cool. I appreciate TypeScript for its debugging ease. There's a lot of work ahead, and I admire your passion and efforts. Thanks for that. I might have more feedback to share over the weekend. Currently, our CI is encountering issues due to an incompatible Node version. The project requires Node v14, as specified in the |
Oops, didn't notice the node change. I might take a look to see if I can rework it for v14 if you thinks that's better. Let me know what you think. |
My apologies for the incorrect message. The Node version isn't the problem, the PR works with Node v14. It's actually a compatibility issue due to a recent npm that breaks the CI. Please rebase the master branch. |
When attempting to fix eslint rules in file, the following error appeared ```Error: "prettier/vue" has been merged into "prettier" in eslint-config-prettier 8.0.0. See: https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md#version-800-2021-02-21``` Reading the linked changelog, it seemed the fix was to simply only use the prettier/recommended extension. Simplified eslintrc to reflect this.
Just did a rebase to include 23f43fd. Hope that is what you meant |
When attempting to fix eslint rules in file, the following error appeared ```Error: "prettier/vue" has been merged into "prettier" in eslint-config-prettier 8.0.0. See: https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md#version-800-2021-02-21``` Reading the linked changelog, it seemed the fix was to simply only use the prettier/recommended extension. Simplified eslintrc to reflect this.
Yes, the CI is passed now. |
Cool. Do you need any more changes? |
No, thanks. Perhaps I will give more feedback on the weekend. |
Do you mind if I do a few more refactors on this PR or would you rather wait on that? |
Sure you can do more refactors. |
Cool, I'll try to keep it at refactors only, or at maximum add more tests. But no new features for now. Ulitmately I'd like to add #49 ;) |
Eslint was complaining about various keywords being reserved and was unerlining them. Fixed by installing `typescript-eslint` according to [this guide](https://typescript-eslint.io/getting-started)
reformat and clean-up using new eslint config
Bumped because >=2 includes typings and seemingly no other breaking changes
On my Windows machine eslint was constantly complaining about line endings. Fixed using this answer https://stackoverflow.com/a/53769213
Add ambient typings for another-npm-registry-client
Move types file into types folder
Based on usage. Could use some cleanup
Declare types instead of exporting them for easier use
Improve type declarations for another-npm-registry-client by reading it's docs
Bump to ES2020 to match Node 14 after consulting https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping
Downgrade to match node version
Add some custom type assertions for expected errors
ts-mocha already compiles TS
Instead of building the package using tsc and then running the js file, we can run using ts-node. That way we only need ts files in the project. For this to work a few changes needed to be done: - Removed the initial empty index file - Moved bin-files into lib - Converted bin files to ts and renamed them to index/index-cn - Changed the shebang in the index files to use ts-node. This also requires ts-node is a dependency of the package - Adjust tsconfig in order to work with ts-node Seems like the package still works as intended. It might take a little bit longer to run because of the additional compile step
Seems like mocha requires it
I hope to have fixed that now. |
Thanks for the rebase. I think my comments have collapsed here: #52 (review), please checkout. |
Yeah sorry, I force pushed and now all the commits are here twice. But I already responded to your feedback. |
@ComradeVanti, sorry I didn't see your response for my previous review #52 (review)? |
@favoyang That's weird. Here is how it looks on my side: |
It says pending. Perhaps there is a button for submitting. Refs https://github.com/orgs/community/discussions/10369
|
Oh god I feel stupid. Thanks, I hope I did it now. |
One more thing, please remove ts-node from the dependencies (or move to devDeps if still needed) and regenerate the package-lock.json. |
Sounds like we're good here. I will give a few more manual tests and merge them tomorrow. |
Cool thanks. I'm a big fan of this project an plan to contribute more. Looking forward to some great collaboration. |
# [1.16.0](1.15.5...1.16.0) (2023-11-01) ### Features * convert project to TypeScript ([#52](#52)), require node v16 ([8e68b9a](8e68b9a)), closes [/github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md#version-800-2021-02-21](https://github.com//github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md/issues/version-800-2021-02-21) [/github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md#version-800-2021-02-21](https://github.com//github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md/issues/version-800-2021-02-21) [/github.com//pull/52#issuecomment-1776779428](https://github.com//github.com/openupm/openupm-cli/pull/52/issues/issuecomment-1776779428) [#50](#50) [/github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md#version-800-2021-02-21](https://github.com//github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md/issues/version-800-2021-02-21) [/github.com//pull/52#issuecomment-1776779428](https://github.com//github.com/openupm/openupm-cli/pull/52/issues/issuecomment-1776779428) [/github.com//pull/52#discussion_r1375481622](https://github.com//github.com/openupm/openupm-cli/pull/52/issues/discussion_r1375481622) [/github.com//pull/52#discussion_r1378008074](https://github.com//github.com/openupm/openupm-cli/pull/52/issues/discussion_r1378008074)
@ComradeVanti thanks again for all the passion. I have added you to the contributor list. |
This is an initial attempt to transition the project to TypeScript.
The tests pass, but obviously there would need to be an additional compile step somewhere when building the project for production.
This conversion has exposed a lot of potential
undefined
and other errors. I have highlighted these withTODO
comments.Hope to get some feedback on this.