PeerPad is not working

[daviddias]

Seems that something got published to PeerPad and it stopped working. Wonder if this was intentional or not, it might have slipped with the auto deploy. //cc @victorbjelkholm

[olizilla]

I'm digging into this now.

[pgte]

@olizilla I couldn't reproduce this locally, but I got the error this._ipfs._libp2p.dialProtocol is not a function.
This PR should fix this: ipfs-shipyard/peer-pad-core#10

[olizilla]

Same here, i think that'll fix it.

[olizilla]

To reduce the scope for dep related issues to appear in prod, and to improve our chances of recreating them locally, we're going to add a package-lock.json spec to the repo.

I'm gonna PR with the minor update to peer-pad-core version, and add the package-lock.json to pin all the deps. I'm then going to look at adding integration tests so CI can give us a heads up about these things before deploying them.

[olizilla]

Ok, so there is a failing test src/components/home/Home.test.js so I'm looking at fixing that too.

[olizilla]

#151 is in the pipe. Awaiting the Jenkins launch codes now.

[olizilla]

Bumping the deps has not fixed the issue. I'll write up what I've found so far, later this eve.

[olizilla]

In Chrome, If you use a fresh browser profile or remove all the indexDbs for the domain, you hit

Uncaught Error: Can't set property: 'links' is immutable
    at module.exports.set links [as links] (0.8008b5f1.chunk.js:69)
    at Object.serialize (0.8008b5f1.chunk.js:69)
    at cb (0.8008b5f1.chunk.js:117)
    at nextTask (0.8008b5f1.chunk.js:33)
    at exports.default (0.8008b5f1.chunk.js:33)
    at IPLDResolver._put (0.8008b5f1.chunk.js:117)
    at IPLDResolver.put (0.8008b5f1.chunk.js:117)
    at DAGNode.create (0.8008b5f1.chunk.js:127)
    at multihashing (0.8008b5f1.chunk.js:69)
    at Multihashing.Multihashing.digest (1.13a6580c.chunk.js:1)

Which looks like ipfs/js-ipfs#1131

[olizilla]

Fix for the can't set links issue was ipfs/aegir#180 which disables mangle and compress in aegir

Recently mangle and compress got re-enabled in a aegir
ipfs/aegir@8d323b7

This is the subsequent error we see in peer pad when refreshing after the initial error

0.8008b5f1.chunk.js:1 Uncaught (in promise) Error: Callback was already called.
    at 0.8008b5f1.chunk.js:1

Which is referenced in
ipfs/aegir#214

So it sounds like something went wrong with the bundling with of js-ipfs for peer-pad-core

[olizilla]

peerpad is currently built as a "website (hugo)" job on jenkins, so its production build is triggered by

sh 'docker run -i -v `pwd`:/site ipfs/ci-websites make -C /site build'

https://github.com/ipfs/jenkins-libs/blob/796cab23030077109f98bbb092d57ed9f4964772/vars/website.groovy#L80

The ipfs/ci-websites docker image is built with these deps https://github.com/ipfs/ci-websites/blob/af0b98f712a5e6bd4174eb86e2ee05c9bdaacb57/Dockerfile

The prod site is the result of calling make build here https://github.com/ipfs-shipyard/peer-pad/blob/8977207042fd41a51e6ce28865e813dab70b4a3c/Makefile#L1-L4

which invokes https://github.com/ipfs-shipyard/peer-pad/blob/8977207042fd41a51e6ce28865e813dab70b4a3c/package.json#L62

which calls the build npm script that runs https://github.com/ipfs-shipyard/peer-pad/blob/8977207042fd41a51e6ce28865e813dab70b4a3c/scripts/build.js and creates an optimised build in the build/ dir of the project, which is what gets deployed.

[olizilla]

No yarn version is specified in the make file

https://github.com/ipfs-shipyard/peer-pad/blob/8977207042fd41a51e6ce28865e813dab70b4a3c/Makefile#L2

To get a deterministic build, we'd need both a yarn.lock file and a pinned yarn version. There is an open PR to pin the version of yarn (and aegir) https://github.com/ipfs/ci-websites/pull/2/files but a project like peerpad should be able to pin its own dependencies and have the build process use them, rather than have CI decide what versions to use.

This points to a need for a "webapp" jenkins build target, that tailored to building single page apps and more complex web apps.

[olizilla]

Using yarn@1.6.0, the same version that CI used (see: https://ci.ipfs.team/job/IPFS%20Shipyard/job/peer-pad/job/master/5/console) and following the same steps

• yarn
• yarn build
• serve up build/ with a static http server

The same errors are observed in the local build

[olizilla]

My preferred solution here is to use npm and a package-lock.jsonfile to pin the deps. The un-answered question is why the dep tree installed by yarn doesn't work but the dep tree installed by npm does...

[olizilla]

@fsdiogo do you have any idea why a yarn build would lead to a bundle with js-ipfs in that exhibits the errors you mentioned in ipfs/aegir#214 but an npm install does not?

[fsdiogo]

Hmm, that's weird.. I don't see any reason for not working with yarn 🤔

[victorb]

This points to a need for a "webapp" jenkins build target, that tailored to building single page apps and more complex web apps.

Indeed! What I want to do with the pipelines is able to pipe them together, so you can call first javascript() and then website() and you'll get testing + deployment.

[olizilla]

Just doing some peerpadding

[one000mph]

@olizilla I'm doing some pm-ing for the andyet team and had a conversation with David this morning. I'd like to understand if a change that one of our devs made caused this issue and how we can avoid it in the future. It is unclear to me whether it was a change in this repo or a pre-existing issue in the CI system or with dependency bundling, that caused peerpad to break. The DEPLOY.md helps to clarify your process so thanks for creating that.

I'm working on getting the the tests for peer-pad to be meaningful, but right now only the end-to-end test in https://github.com/ipfs-shipyard/peer-pad/blob/master/test/e2e/smoke.test.js asserts anything useful.

The tests are failing on this PR as it changes a Jest snapshot. I've just tried it locally, and I think you just need to revert the change to the snapshot and the tests will pass.

It looks like the CI tests than run on a merge are not entirely stable and from your comment above it sounds like we should be running the some tests manually prior to merges since deploy happens automatically on merges to master. Can you please clarify the testing process we should be following, or if you prefer to leave PRs in features branches until more thorough testing exists? Thanks!

[olizilla]

@one000mph I'm a little uncomfortable. I feel like I'm missing some nuances. Please can we arrange a call? It'd be great to talk with you all.

The short version is, no, as far as I can tell, none of your devs caused this issue. Work was done by PL to allow peerpad to be automatically deployed to production when commits are merged to master. The build process that was used to create the prod site used yarn to install the deps and build the optimised site. For reasons I've not tracked down yet, building the site with yarn lead to the errors in production. I was able to recreate the issue locally, and following the same steps with npm instead of yarn did not exhibit the same problem. The 'fix' wasn't really satisfactory as I simply changed the Makefile that CI uses to build the site to use npm, which fixed the issue, but I still need to figure out why.

Other problems that this event uncovered are

• There was no lockfile in the repo. Had the issue been a problem with a specific dependency tree that had been resolved on CI some days ago, we wouldn't have been able to reliably recreate that dep tree to help debug it. There is now a package-lock.json in the repo, and npm should be used to build the app.

• There are no useful unit tests. This wouldn't have prevented this issue, but it is something that want, so we can improve the robustness of the app as it grows.

• The 2 existing unit tests simply test that a couple of react components can be instantiated and mounted in a fake dom without error. They don't really test anything interesting, but also cause dev friction as they rely on jest snapshots to assert that they keep rendering the same dom structure from run to run. When a dev changes the structure of the dom deliberately they have to remember to update the jest snapshots, which currently just feels like friction as changes to most of the codebase won't trigger that issue, so it comes as a surprise when you do change something deliberatly and get "snapshots don't match yo!" error. On the plus side, the test error includes instructions on how to update the snapshots. It's questionable how much value a snapshot test provides on a new app that is likely to have significant ui changes, so it'd be great to get your teams feelings on that.

• There isn't currently a clear lead maintainer for peer-pad. It was built as demo and then people got busy with other things. I happened to see the complaint that led to this issue and was available to debug it.

Can you please clarify the testing process

The CI tests are stable, we need more of them. The e2e smoke test will at least tell us if users can create new pads without an error, and sync 2 pads via IPFS. If CI says that isn't working, it needs to be checked out before the PR is merged. Of note, that is a full on integration test, nothing is stubbed out, so it can be affected by network or IPFS infrastructure issues, but the point is, if it does fail, for any reason, then it's likely it will also fail for users, so is worth checking.

PRs should be reviewed by at least 1 other person before merging to master, the general guidelines that we're converging on for all-the-things are here https://github.com/ipfs/community/blob/master/js-code-guidelines.md

[olizilla]

Also of note, it occurs to me that smoke test being run on travis won't catch the issue of the travis test build steps diverging from the jenkins deploy build steps, which is the root cause of this issue.

@victorbjelkholm it'd be rad if we could add a post build test hook to jenkins that could pass the root hash of the site once it's been added to ipfs but before we update the dns. We could configure the smoke test to run against the ipfs.io/ipfs/QmLatestHash which would also catch issues like non-relative urls used for assets as found in #154