-
Notifications
You must be signed in to change notification settings - Fork 44
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
v8.1.0 Update #86
base: master
Are you sure you want to change the base?
v8.1.0 Update #86
Conversation
* Emit types using tsc
* Update deprecated code
* move lib to src * path updates - as suggested by @johnhooks to set as source for tests the "src" folder (and not the builded one) - updated node internal modules paths prefix (ref. https://nodejs.org/api/module.html#modules-nodemodule-api) * typescript file compilation config
* speed up lexer parsing
- scripts - make sure that all is ready before fire the script
@erikyo thank you for your efforts! |
@smhg, I've organized the pull requests (PRs) in the following manner: each individual PR that has been merged into this branch encompasses all the 'one-shot' modifications made to the repository for the next version. PR #89 addresses the correction of JSdocs and typos. Although you can navigate to the link to inspect each PR individually and check their respective changes, I've been gradually merging them into the development branch to preemptively address potential conflicts during the merge process. However, if any critical issues arise, please bring them to my attention, and I'll promptly rectify them. Consider this branch as a pre-publication quality control checkpoint. I've included you in this process so that you have the "last word" over it. |
@erikyo maybe I don't have the git skills required to understand your approach/workflow. |
I understand your concerns, and I appreciate your feedback. Let me clarify my approach and workflow regarding the changes I've proposed. The reason why you didn't see those changes in the master branch, is because I prefer to keep changes in a branch concurrent with the master branch. This has a simple reason... users that needs to download (or check the code) from the github repository should do so with the code from the latest release and not the code in development (which may not be stable). At the moment I'm still missing a set of modifications (#89 which is now a draft) because I'd like @johnhooks to check it too, that guy is really good at this and his contribute would be really helpful About typescript: The current changes primarily focus on stabilizing and updating the repository, rather than introducing TypeScript-related updates. The TypeScript compiler (tsc) is currently utilized solely for generating types and compiling JavaScript. This is why you'll notice In summary, I intend to delay merging any changes to the master branch until I've completed the entire set of modifications I've outlined. This allows us to maintain a clear separation between development and stable code (if you want to try the development version, however, just checkout the development instead of the master and you will be on top of all the new updates). I hope this clarifies my approach, but I welcome any further discussion or suggestions you may have.
Yes this you are right, this kind of change could be done directly on the master, and not the kind of changes to put on the development branch stack. I will try to solve this as soon as possible! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikyo the details are in the comments, though my biggest question: why manually create a type declaration, rather than use one generated by tsc
? It looks like you have it configured in the tsconfig.json
for it to generate a declaration file.
I've read through the previous comments for this PR. I agree with @smhg, it would be best to create separate PRs for the following:
I also don’t see a reason to keep the main branch in sync with releases. That is the purpose of git tags and GitHub releases. Though it all depends on the workflow preferred by the maintainer. |
Currently, there are 400 errors in types, and the generated output doesn't match our expectations. While your suggestion would be ideal, attempting to generate the file entirely now yields unexpected results. 👆 and since many types aren't defined they becomes "any" (which basically means not typed) To address this issue fully, it necessitates either making very radical changes (or submitting several PRs). This partly explains the purpose of the dev branch (Release Branch Strategy). The rationale behind it is that the required changes are too extensive to 'fix everything' at once, yet an interim version cannot be published to the master branch. |
In my opinion, types aren't currently published by the package,so it doesn't make sense to copy the Definitely Typed ones. Those are established and what people are used to. It would be best to wait and publish the actual types when they are ready. I wouldn't copy the types from Definitely Typed, but enable types with very low strictness, and start improving the situation in increments. The types can be incomplete and unpublished (by unpublished, I mean not adding the |
Sorry John, maybe I didn't provide enough context. The types from Definitely Typed are for version 4.0.4 and not for the current 8.0.0, and they are indeed very generic, sometimes incorrect (like in the case of comments where they are all marked as required when they're not) and they only cover the exported functions in the index. When I mentioned using the same types, I was referring to the naming convention chosen for the translation block, whether it's "GetTextTranslation" or "GetTextTranslations". So, #89 isn't a copy of Definitely Typed, but it's obviously very similar since the functions and types are the same (as I mentioned earlier, even the names). However, they are correct, which isn't the case for Definitely Typed. I'm taking steps forward: in #81, I started generating types (with some issues because index.js and index.d.ts were in the root), then in #85, I moved all the files into src (which resolved the problem), etc. Making it strictly typed requires changing or adding a lot of code, unfortunately not all JSDoc comments were correct, and this results in many lines to be changed, and consequently, if not one giant PR, then many small PRs (which is what I'm attempting to do). |
Thanks @erikyo for the explanation, I'd would still lean toward waiting to publish them until they are "correct" and generated by properly typed code. IMHO @smhg Could we please enable discussions for this repository? I think that could be a better place to hash out the details of git strategy, rather than in a PR or Issue |
@johnhooks After a bit of tweaking yesterday I managed to reduce the errors to 280, so I don't think the result is that far off. you should try the #89 using |
@johnhooks I've started one discussion topic. Hopefully that does the trick. Please tell me if this isn't set up properly. |
* fix missing types and jsDocs * removing definition file, the definitions were moved to the type.js file * enhanced type for pocompiler and postream * enhancing js docs types * jsdocs types (still few error to solve) * @johnhooks review suggestions Co-authored-by: John Hooks <[email protected]> * apply suggestion by johnhooks Co-authored-by: John Hooks <[email protected]> * apply @johnhooks suggestions 🙌 * wip types * fixed types * allows tsc to fail in ci tests * fix: adjust typing of the parsers and compilers This commit adds missing types and attempts fix type errors. There are still a few type errors, though how to fix them is not clear. Adds the `Translations` type for the `translations` property of the `GetTextTranslations` type. * add imports for types * add encoding declaration * add types directory to tsconfig include * remove types directory from .gitignore --------- Co-authored-by: John Hooks <[email protected]>
* track coverage using c8 * Fix: tests runs twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good. I found just a few extras in a JSDocs.
* @param {Size} size Byte size information | ||
* @return {Buffer} Compiled MO object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove these spaces... also would be nice to add a JSDoc eslint rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep you are right, it seems that jsdocs aren't formatted at all.
What do you suggest to fix that issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikyo I have a branch that is almost done, in which I manually format them. I looked into using eslint-plugin-jsdoc
though I was having some trouble because eslint-config-standard
hasn't be updated (by it's maintainers) and I would need to figure out what version of eslint-plugin-jsdoc
should be added as a dev dependency. If there is support for adding the JSDoc plugin, I can figure it out but didn't want to commit much time to it, if there wasn't.
this is the PR that contains all the changes listed in #82
There are no dramatic changes, however, we may decide to move to 8.1.0 (and not 8.0.1) because of the modified type system, which did not require immediate action, but it would be preferable to suggest to remove the "definitively typed" types (if someone is using ts). For that reason a changelog file update is nice, hopefully someone is using
npm outdated
NOT ready to merge