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

Merge js-libp2p interfaces into one repo #383

Closed
26 tasks done
daviddias opened this issue Jul 20, 2019 · 13 comments
Closed
26 tasks done

Merge js-libp2p interfaces into one repo #383

daviddias opened this issue Jul 20, 2019 · 13 comments
Assignees
Labels
dx Developer Experience Epic exp/expert Having worked on the specific codebase is important P1 High: Likely tackled by core team if no one steps up status/in-progress In progress

Comments

@daviddias
Copy link
Member

daviddias commented Jul 20, 2019

Following ipfs/js-ipfs#2222

Can I get a Yay or block on this? Happy to take this one and ship it next week

Modules

Cleanup

NPM Deprecation Notice

Readme Deprecation Notice

Github Archival

Description replacement: [DEPRECATED]: now part of the https://github.com/libp2p/js-interfaces repo

Github Archival: Record Store

Description replacement: [DEPRECATED]: deprecated in favor of https://github.com/ipfs/interface-datastore

@vasco-santos
Copy link
Member

I totally agree on this. Just thinking if we should finish the async migration for all the interfaces before. Now we have a few with async await, while others with PRs for async await on WIP

@daviddias
Copy link
Member Author

Fair point. What is your prediction for that endeavor to be finished?

@jacobheun
Copy link
Contributor

jacobheun commented Jul 25, 2019

The number of PRs is actually pretty small, so I don't think we need to wait. There may be some coordination annoyances deploying things individually, but it's easy enough to recreate them in the new repo.

I started a new repo, currently under my org for experimenting but I will move that over to libp2p soon, https://github.com/jacobheun/js-interfaces.

Edit: We can ignore the below per ipfs/js-ipfs#2222 (comment). The interfaces will get released as a single module, libp2p-interfaces.


The major thing I have realized after moving these all under 1 repo is that releasing them individually with Aegir would be annoying. Tagging is the main reason. If the modules are still released individually, Aegir will want to create tags for them, which will likely conflict with the tags from the other interfaces. To get around this we could look at updating Aegir to be able to specify prefixes for tags, or possibly a sub path of the folder you want to release.

This could look like:

aegir release -t node --dir stream-muxer

which would look at the package.json file there, and handle any version bumping. The tag could then be prefixed with the path, stream-muxer/v0.6.1 in this instance.

Thinking Forward

For the interfaces I think we could really just combine them and release as a new package libp2p-interfaces, and deprecate the old ones, but I think we can have that conversation separately.

@jacobheun jacobheun self-assigned this Jul 25, 2019
@jacobheun jacobheun added the status/in-progress In progress label Jul 25, 2019
@jacobheun
Copy link
Contributor

jacobheun commented Jul 26, 2019

Continuing the conversation from ipfs/js-ipfs#2222 (comment) here, as it pertains primarily to this migration.

We just need to require specific interfaces where needed to avoid bundle problems. Just like we do for lodash and pullstreams.
There's no need to require a global index.js that brings everything in fact we shouldn't even have a index.js that re-exports the interfaces, just one file per interface.

Right, so anyone bundling should be fine as only the needed dependencies should get bundled, however if you're not bundling you may get some extraneous dependencies, like chai because it is currently a dependency of the upcoming libp2p-connection

@hugomrdias another thing I am thinking about is how the test suites and interfaces themselves will be required. For example, let's say I want to use the Transport interface for one of the async migrations. It has some base Error exports, along with the Transport test suite. My thinking is we would do something like:

// our test file
const TransportTests = require('lib2p-interfaces/transport/tests')
// In the src transport code
const { AbortError } = require('lib2p-interfaces/transport/errors')

Our current setup, has us nesting all source code in ./src, which makes these requires actually look like require('lib2p-interfaces/src/transport/tests') and require('lib2p-interfaces/src/transport/errors') which isn't a huge issue, but I'd rather us not use /src in our require paths as it's an antipattern. Lodash avoids this by keeping all files at the root level and they build the main file as part of the deploy so you can do const _ = require('lodash'). Async.js handles this with a build script that compiles ./lib files into the root of the deployed package allowing require('async/parallel') instead of require('async/lib/parallel').

I am trying to think through how we could have aegir handle some of this and/or through repo structure, without causing issues with the webpack builds.

@hugomrdias
Copy link
Member

@jacobheun the problem here is the /src in the require path ?
i don't see this as a problem really, our source code doesn't need transpiling which is really good so the source is useable (i don't think we gonna ever change that) !! Plus the user only needs the repo to understand the file structure.

IMO this is a js community thing that tends to over complicate everything, i don't think we should buy into it.

That said if the js team thinks this is the way to go, we can add an extra step to the release command to copy /src/* to whatever folder.

@daviddias
Copy link
Member Author

Either way, I think both ways will be fine. The most important is to coalesce the interface repos for improved developer productivity :)

@jacobheun
Copy link
Contributor

I have created a PR for the merge at js-interfaces#1. I also updated the original post here to include steps for cleanup, once that has been released.

@daviddias
Copy link
Member Author

Done, right?

@vasco-santos
Copy link
Member

I created PRs for adding the deprecation notice in the README. Once we release 0.27 we can merge them all, add npm deprecation and archive repos

@vasco-santos
Copy link
Member

FYI, we need to transfer the issues and somehow track the open PRs that might be relevant and close the others.

@daviddias
Copy link
Member Author

@vasco-santos I moved the issues. Didn't find any open PRs (but wouldn't be able to move them if there were any)

@vasco-santos
Copy link
Member

Thanks @daviddias ❤️

@jacobheun jacobheun added exp/expert Having worked on the specific codebase is important dx Developer Experience Epic P1 High: Likely tackled by core team if no one steps up labels Mar 11, 2020
@jacobheun
Copy link
Contributor

  • All readme's have been updated.
  • All github repos have been archived.
  • npm deprecation notices have been added to all versions of the modules pointing to the libp2p-interfaces module.

This is done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx Developer Experience Epic exp/expert Having worked on the specific codebase is important P1 High: Likely tackled by core team if no one steps up status/in-progress In progress
Projects
None yet
Development

No branches or pull requests

4 participants