Skip to content

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Jul 11, 2022

  • The latest fs-admin was updated to use Node-API instead of NAN. 🎉
  • Node-API is better than NAN. 🥇
  • We are still using an older version of fs-admin. 🕚
  • We should update to newer fs-admin that's based on Node-API. ⬆️

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jul 11, 2022

Note: I used fs-admin 0.19.0 instead of 0.20.0 in core. That's so we can dedupe it with the text-buffer package's indirect dependency on fs-admin 0.19.x.

They are very similar versions of fs-admin. Only two source code lines changed, and two dependency versions were bumped. See: atom/fs-admin@v0.19.0...v0.20.0

We can either accept my PR as-is (deduped fs-admin 0.19.0), accept two different versions of fs-admin in different parts of Atom, or fork text-buffer to bump its fs-admin dependency to 0.20.0. (Or do that last option some time later and merge my PR for now.) Just thought I would mention it and explain myself.

Copy link
Member

@aminya aminya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! We should also update the text-buffer. I remember making some pull requests that did not merge.

Consistent fs-admin version in script/ and in core.
Copy link

@icecream17 icecream17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will wait for consensus

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jul 12, 2022

Yeah, wasn't sure if it is supposed to be all 0.19.0 or all 0.20.0, but I can only do all 0.19.0 at the moment.

So, it's all 0.19.0 now. If that's what was requested, then this is ready to go ✔️.

@aminya aminya merged commit 7501657 into atom-community:master Jul 12, 2022
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.

3 participants