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

node_io: Implement FileSystem from package:file #86

Closed
jakemac53 opened this issue Nov 10, 2020 · 10 comments · Fixed by #88
Closed

node_io: Implement FileSystem from package:file #86

jakemac53 opened this issue Nov 10, 2020 · 10 comments · Fixed by #88

Comments

@jakemac53
Copy link
Contributor

Related to dart-lang/tools#1991.

Today the glob package uses node_io with a conditional export but we want to get away from that. We are switching to use the file package for all file system interactions.

If this package had an implementation of FileSystem, then it could easily plug in to glob in a clean way. This might also help some of the other issues listed for this package, if we can push more of the ecosystem towards using these general file interfaces instead of those from dart:io, then everything can be more flexible :).

@pulyaevskiy
Copy link
Owner

First thought that comes to my mind is that we can create node_file package which implements package:file interface and relies on node_io internally. It'd be a fairly slim implementation and would allow to keep node_io in its original state intended to match dart:io interface.

Would this be a plausible solution to address the issue with glob package?

@jakemac53
Copy link
Contributor Author

Yep, that would work as well

@jakemac53
Copy link
Contributor Author

jakemac53 commented Nov 16, 2020

One thing I realized with this is that it looks like node_io does actually depend on dart:io (it implements the types).

That might not always continue to be allowed by the compilers, not sure.

@nex3
Copy link
Collaborator

nex3 commented Nov 17, 2020

One thing I realized with this is that it looks like node_io does actually depend on dart:io (it implements the types).

That might not always continue to be allowed by the compilers, not sure.

Hopefully such a major breaking change would come with plenty of warning and migration assistance.

Incidentally, the file package also depends on dart:io for some of its interfaces.

@nex3
Copy link
Collaborator

nex3 commented Nov 17, 2020

@pulyaevskiy I'm diving into this to unblock sass_migrator moving to the latest version of glob, and I've come to a crossroads where we could go one of two ways:

  1. As you mentioned above, we could create a new node_file package that implements the FileSystem API as a wrapper around node_io. The upside of this is a cleaner separation of concerns, but the downside is that you have an extra unnecessary wrapper layer despite the fact that the APIs between the two are almost identical (since FileSystem's types all also implement dart:io's).

  2. We could modify the existing node_io types to support the few extra members that FileSystem adds (such as Directory.childDirectory()). The downside of course is that these APIs would then have a few members that aren't present on dart:io, but the upside would be a substantially simpler implementation of FileSystem that's likely to be easier to maintain going forward as well.

I'm happy to implement either, but I'd like to know which direction you think is preferable. My personal preference would be for option 2, due to its simplicity.

nex3 added a commit to nex3/node-interop that referenced this issue Nov 17, 2020
@nex3
Copy link
Collaborator

nex3 commented Nov 17, 2020

Option 2 ended up being simple enough that I just put up a draft PR for it: #88

@pulyaevskiy
Copy link
Owner

but the downside is that you have an extra unnecessary wrapper layer despite the fact that the APIs between the two are almost identical

Agree, it's adding a lot of code with little value.

With option 2 I was worried that the two APIs may introduce incompatible changes at some point in the future which would break this package. But it looks like the file package types also implement dart:io's interfaces and will have to follow those anyway.

So 👍 for option 2. Thanks for taking time to work on this @nex3 ! I glanced briefly at dart-lang/glob#88 and it looks good overall, let me know when you're done and it's ready for review.

nex3 added a commit to nex3/node-interop that referenced this issue Nov 18, 2020
nex3 added a commit to nex3/node-interop that referenced this issue Nov 18, 2020
@nex3
Copy link
Collaborator

nex3 commented Nov 18, 2020

dart-lang/glob#88 is ready for review (although it's intended to land after dart-lang/glob#87).

@pulyaevskiy
Copy link
Owner

Thanks, I merged dart-lang/glob#87, will look at dart-lang/glob#88 tomorrow.

pulyaevskiy pushed a commit that referenced this issue Nov 21, 2020
* Add helpers for invoking asynchronous Node.js functions

* Implement file's FileSystem API in node_io

Closes #86

* Remove dependency override
@pulyaevskiy
Copy link
Owner

Thanks @nex3 , I published node_io 1.2.0 and node_interop 1.2.1. (Also fixed the build while at it).

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 a pull request may close this issue.

3 participants