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

Modernising to single package with esm and types #84

Open
daKmoR opened this issue Sep 13, 2021 · 15 comments
Open

Modernising to single package with esm and types #84

daKmoR opened this issue Sep 13, 2021 · 15 comments

Comments

@daKmoR
Copy link

daKmoR commented Sep 13, 2021

hey there 👋

I was wondering if there would be any interest in "updating" the code to esm?
I'm also very conscious of the number of dependencies and I think we could get that down to 0?
I also noticed some typing issues for children...

anyways - I could do all the work and do esm, 0 dependencies, and improving types... would you like to release it maybe as the next major version?
or would you prefer me to fork?

@joaonuno
Copy link
Owner

joaonuno commented Sep 14, 2021

Hey! I like your suggestions and would appreciate your contributions. I can't commit much time to this so please break it down in separate MRs to make it more digestible.
I'm not familiar with how much breaking is changing the distributed module type but I guess a new major makes sense.
Regarding number of dependencies, I'm not strict on that, actually the two dependencies are also mine but I agree we should get rid of them because:

  • merge-sort-js was built just to provide stable sorting (when the lib was developed the JS sort method wasn't stable but I think it is now both in node and browsers, please confirm this)
  • find-insert-index-js is doing a linear search when it could do a binary search (not sure if there's anything native to replace this)

I'm not a TS user so I don't know what's missing or needs improvements in regard with typing. If it's possible to make those improvements separately and released as a new patch version that would be desirable.

I also ask you to avoid changing code and tests in the same MR to keep it more controlled in terms of breaking changes.

@daKmoR
Copy link
Author

daKmoR commented Sep 14, 2021

sounds good 👍

First PR will be about moving to ESM and classes which should mostly be syntactical changes while hardly touching logical code. Just to be sure this will raise the requirement to node 14+ (current active LTS)

we can then align on how to take the next steps 🤗

@joaonuno
Copy link
Owner

joaonuno commented Sep 14, 2021

Just to be sure this will raise the requirement to node 14+ (current active LTS)

Did you check if Node 12 (still receiving security updates) already supports ESM? If not, or if it's incomplete, then do bump the minimum required to 14.

@daKmoR
Copy link
Author

daKmoR commented Sep 14, 2021

Suggested steps

  • move to esm (PR)
  • keep working in a branch? (do next PRs to it? there are more changes needed)
  • format with prettier
  • add github action to automatically run tests in PRs
  • split into src/TreeModel.js, src/Node.js, src/walkStrategies.js (PR)
  • remove dtslint, jshint
  • update eslint
  • evaluate what would be needed to become a package with 0 dependencies
  • add typescript and auto-generate types based on jsdoc

@daKmoR
Copy link
Author

daKmoR commented Sep 14, 2021

Node 12 indeed also supports esm but IIRC it does not support export maps which I would also like to add

@daKmoR
Copy link
Author

daKmoR commented Sep 14, 2021

@joaonuno thx for the approval 🤗

how would you like to proceed?

suggestion:

  1. merge the open PR into another branch like next
  2. you would need to create and push this next branch before I can choose it as a target
  3. all following merge requests would target next
  4. ... work work work
  5. after all steps are done merge next into master and released it as 2.0.0 (potentially doing some pre-releases before)

@joaonuno
Copy link
Owner

Thanks for the suggestion. Just created next branch.

@daKmoR
Copy link
Author

daKmoR commented Sep 17, 2021

hey there 👋
any chance you can merge #85 so I can make further PRs on the next branch?

@joaonuno
Copy link
Owner

Done, sorry for the delay.

@daKmoR
Copy link
Author

daKmoR commented Oct 5, 2021

I got pulled in some other tasks 🙈 but I'm back... finished "remove dtslint, jshint" and "update eslint" via #89

I brought those tasks forward as while splitting the files I noted I didn't get any hints about missing imports or functions... so I want that fixed first... as it will make the work easier

@joaonuno
Copy link
Owner

joaonuno commented Oct 7, 2021

Hi,
Was wondering if, when you mentioned that you're planning some type improvements, you also noticed the potential improvement mentioned in #88. If so, maybe it's something that can enter in the next major. WDYT?

@daKmoR
Copy link
Author

daKmoR commented Oct 7, 2021

yes - I assume this will come up once we add type checking for js files

just finished splitting it into multiple smaller parts in #90

I think these are the bigger remaining open tasks

  • evaluate what would be needed to become a package with 0 dependencies
  • add typescript and auto-generate types based on jsdoc
  • Make it type module and rename all mjs to js
  • Beta Release preparation (export map, readme, changelog, ...)

@daKmoR
Copy link
Author

daKmoR commented Oct 12, 2021

can I ask you to review/merge #90? 🙏

@daKmoR
Copy link
Author

daKmoR commented Oct 14, 2021

here we are - 0 dependencies 💪 see #91

@daKmoR
Copy link
Author

daKmoR commented Oct 25, 2021

yeah, huge changes in

what do you think? how do you want to proceed?

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

No branches or pull requests

2 participants