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

v8.1.0 Update #86

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

v8.1.0 Update #86

wants to merge 8 commits into from

Conversation

erikyo
Copy link
Collaborator

@erikyo erikyo commented May 4, 2024

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

erikyo added 3 commits May 4, 2024 23:54
* 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
@erikyo erikyo mentioned this pull request May 4, 2024
3 tasks
@erikyo erikyo linked an issue May 4, 2024 that may be closed by this pull request
10 tasks
@erikyo erikyo requested a review from smhg May 5, 2024 19:12
- scripts - make sure that all is ready before fire the script
@erikyo erikyo changed the title Update n.e.x.t version v8.1.0 Update May 5, 2024
@smhg
Copy link
Owner

smhg commented May 6, 2024

@erikyo thank you for your efforts!
If you want me to review these changes, please make it easy and separate things into separate PR's. Like i've been requesting.
Please put all cosmetic changes (imports, directory structure, typos,...) into one PR. I'm (by now) very happy to quickly review that.
What then remains is only a limited set of changes related to TS which is harder for me to look at and the separation makes that review too much easier.

@erikyo
Copy link
Collaborator Author

erikyo commented May 6, 2024

@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.
PR #81 focuses on type checking.
PRs #84 and #90 deal with the replacement of deprecated elements.
PR #85 involves the relocation of all files into the 'src' directory.
PRs #86 and #87 optimize performance and eliminate a dependency.
For a detailed explanation, please refer to this link.

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.

@smhg
Copy link
Owner

smhg commented May 8, 2024

@erikyo maybe I don't have the git skills required to understand your approach/workflow.
To pick just one of the changes: I expected to see a PR that applies renaming /lib to /src being applied to the code currently in master.
Instead, if I look at #85 I see changes to tsconfig.json file and a types property in package.json. Both don't exist in master. It doesn't really matter this is applied in a development branch, right? It is not possible to consider these changes separately (now and in the future) because they seem to be applied after TS related changes?
Additionally, you can argue the changes in that PR related to import statements should also rather be applied separately. But I'd be fine with one PR for all these 'meta' changes before anything related to TS.
Does what I'm suggesting make any sense to you? Or just tell me to shut up :)

@erikyo
Copy link
Collaborator Author

erikyo commented May 8, 2024

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 this stage, i think it's better to keep these modifications on the dev branch, and this approach ensures that we can maintain a stable master branch until all planned modifications are finalized.

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 allowJs: true in the tsconfig.json.

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.


about the changes in that PR related to import statements

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!

Copy link

@johnhooks johnhooks left a 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.

src/index.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@johnhooks
Copy link

johnhooks commented May 8, 2024

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:

  • Moving lib to src.

  • Updating of dependencies

  • Refactoring the usage of old APIs.

  • Converting away from regex for finding comments and quotes.

  • Adding the initial generation of type declarations.

    For this issue, I don't see a huge benefit in implementing the types in JSDocs, if there is support for converting to TypeScript.

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.

@johnhooks johnhooks mentioned this pull request May 8, 2024
10 tasks
@erikyo
Copy link
Collaborator Author

erikyo commented May 8, 2024

why manually create a type declaration, rather than use one generated by tsc?

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.

image

👆 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.

@johnhooks
Copy link

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 "types" property to the package.json until they are ready). This would not require a full overhaul of the package in one large step. A build step can still be used to convert ts to js files. Flip them all to ts as they are and accept they will not pass a strict type check, or any type check, but then it can be worked at in digestible chunks.

@erikyo
Copy link
Collaborator Author

erikyo commented May 9, 2024

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).

@johnhooks
Copy link

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

@erikyo
Copy link
Collaborator Author

erikyo commented May 9, 2024

@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 "checkJs": true in tsconfig you would have a clearer situation 😉

@smhg
Copy link
Owner

smhg commented May 9, 2024

@johnhooks I've started one discussion topic. Hopefully that does the trick. Please tell me if this isn't set up properly.

erikyo and others added 2 commits May 12, 2024 21:02
* 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
Copy link

@johnhooks johnhooks left a 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.

Comment on lines +226 to +227
* @param {Size} size Byte size information
* @return {Buffer} Compiled MO object

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.

Copy link
Collaborator Author

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?

Copy link

@johnhooks johnhooks May 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repository Refactoring (proposal)
3 participants