Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

[WIP] feat: use pull stream based mplex #1346

Closed
wants to merge 2 commits into from
Closed

[WIP] feat: use pull stream based mplex #1346

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad45349
feat: used pull stream based mplex
dryajov May 9, 2018
c2baa0a
fix: use ~ for 0 versions
dryajov May 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,13 @@
"joi": "^13.2.0",
"joi-browser": "^13.0.1",
"joi-multiaddr": "^2.0.0",
"libp2p": "~0.20.4",
"libp2p": "libp2p/js-libp2p#feat/pull-mplex",
Copy link
Member

Choose a reason for hiding this comment

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

This change should not be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was specifically to test this changes on CI. Not to be merged.

Copy link
Member

Choose a reason for hiding this comment

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

@dryajov Jenkins builds pushed branches, you don't need to submit a PR also.

Is this safe to close this PR or are you going to update the branch with the new version number when published?

For the future it would be rad to get an indication that the PR is a work in progress and/or shouldn't be merged yet. I think people have been putting "WIP" in the PR title for this - maybe this should go in the guidelines?

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanshaw @diasdavid I've added the WIP tag. This should not be merged at all, or at least not until the branch I'm pointing to in the git url gets released as a version, at which point we would need to update the git url to the required version.

Jenkins builds pushed branches, you don't need to submit a PR also.

I'm not sure what you mean here?

What I'm trying to accomplish here:

The idea with this PR (and similar) is to be able to tests changes deep in the dependency tree across different modules. AFAIK, so far we don't have a clear approach to do that and our practice has been to locally link and tests everything. This IMO is incomplete, because it confines our testing to the local environment as well as hides/exposes issues specific to linking.

IMO, it would be super cool to be able to test our changes in CI before releasing a final version and not only locally. This allows us to tests the changes in an isolated environment without relying on linking, across multiple runtimes and OSs. This is what I'm trying to do here.

There are two approaches that I know of to accomplish this:

  • One is releasing a dev/beta/alpha version suffix with a build number e.g. - 0.0.1-alpha.1
    • this of course requires publishing a new version (to either NPM or a local repo) for each build every time
  • Another is using git urls that point to npm packages
    • this is the approach I took, IMO this is the least invasive and error prone approach (based on my experience)

Copy link
Member

Choose a reason for hiding this comment

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

Jenkins builds pushed branches, you don't need to submit a PR also.

I'm not sure what you mean here?

I mean, if you push your branch to this repo Jenkins will build it. I did this recently with fix/dns. I didn't open a PR because it isn't finished yet, but Jenkins built it here: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfs/detail/fix%2Fdns/2/pipeline/20/

Copy link
Member

Choose a reason for hiding this comment

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

Speaking about testing dependencies in Jenkins, I've added a feature of adding custom node modules that will override the modules defined in package.json, so you can test dependencies via CI without having to change package.json. My worry about this feature is missing to release things before merging, leading to broken modules. See https://github.com/ipfs/jenkins-libs#example-2

We can have something flag repos with a custom Jenkinsfile, as none of the JS ones should have a custom one really.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanshaw ah, thanks - didn't know that. I still think the PR doesn't hurt at all, since it gives it visibility and a place to discuss things if/when they arise while running the tests. I think the lack of the [WIP] tag is where most of the confusion came from. Apologies for that.

@victorbjelkholm ah, thats a cool feature - I'll keep it in mind.

My worry about this feature is missing to release things before merging, leading to broken modules.

I'm not sure if this increases the changes for us to releasing broken modules, maybe I'm missing something, but that should not make our review/release filters. If anything, we can add a point to our release checklist to check for this specifically?

In any case, I'm glad we're having this discussion and I'm open to suggestions for different/better approaches 👍

"libp2p-circuit": "~0.2.0",
"libp2p-floodsub": "~0.15.0",
"libp2p-kad-dht": "~0.10.0",
"libp2p-keychain": "~0.3.1",
"libp2p-mdns": "~0.11.0",
"libp2p-mplex": "~0.7.0",
"libp2p-mplex": "libp2p/js-libp2p-mplex#feat/pull-stream",
"libp2p-railing": "~0.8.1",
"libp2p-secio": "~0.10.0",
"libp2p-tcp": "~0.12.0",
Expand Down