Browserify src

[alanshaw]

This PR adds browserify to the build process so that modules and dependencies can be more easily managed using require. I wanted to see if this is desirable and if so, if what I've done is heading in the expected direction!

• Removes the need to manually copy npm dependencies
• Allows us to explicitly define libraries a script depends on at the top, instead of having to ensure script tags are present on the page that load it's dependencies (fewer mystical globals, fewer eslint-disable-lines for "unused" functions/objects, fewer /* global foo */ declarations)
• ipfs-companion.js now has to attach exposed functions/objects to the window, which is great because it is more explicit about what it provides to the other pages in the extension
• Uses factor-bundle to create a common.js with dependencies shared by multiple components (so we're not including multiple versions of dependencies in the build)

Some thoughts:

• We should probably move the add-on/src directory to the top level and build into add-on so we don't package it in the zip
• From here we can start to split ipfs-companion.js up into modules (the section comments in the file are a good indicator of what modules we could pull out). This will make it easier to test and understand, since it's grown really big!

[olizilla]

@alanshaw yep. tested and works in chrome, loaded as an unpacked extension.

I'm +1 for browserify, I find it much easier to reason about than webpack, or globals. It's worth noting that the dev process would involve running npm run watch as the src dir would not longer be runnable directly in dev, but I feel being able to keep things modular is worth it.

Reproducible builds are a requirement, see #306. We need to check that pinning the version of browserify is enough to ensure we get the same output on different systems. Aside, there is work to do to make js-ipfs-api builds reproducible see: ipfs-inactive/js-ipfs-http-client#626

[olizilla]

@alanshaw https://developer.mozilla.org/en-US/Add-ons/Source_Code_Submission is worth a read.

[alanshaw]

Yes, so, as you know, pinning your dependencies only ensures that you get the same version of that specific dependency, not it's dependencies, or it's dependency's dependencies, and so on.

To get a reproducible build we probably need to add a package-lock or shrinkwrap. I did originally add a package-lock, but I considered it overreaching for this PR so removed it. However, thinking about it now, that was wrong, since we'd no longer be using the packaged *.min.js files from our dependencies (which would effectively be a locked version if a dependency is pinned). I will add it back.

[lidel]

Thank you for pushing this @alanshaw! This change was long time due 👍

We should probably move the add-on/src directory to the top level and build into add-on so we don't package it in the zip

It should be possible to run web-ext with --ignore-files add-on/src/**

From here we can start to split ipfs-companion.js up into modules (the section comments in the file are a good indicator of what modules we could pull out).

Yup, that was the plan all along. It is exciting we are getting closer to that reality! 🎉
But let's keep this PR small and sweet -- split can be done in separate PR(s) after this one is merged.

▶️ Some things to address before we can merge this PR (already mentioned by @olizilla 🙂 ):

• Our tiny (but growing) test suite should pass (npm test)
• Browserified build should be reproducible (see Reproducible Build #306 for more details), which means you probably need to:
  • change npm run build to execute in isolated, deterministic environment (see example in [WIP] Reproducible builds ipfs-inactive/js-ipfs-http-client#626 (comment))
    • remove package-lock.json and add it to .gitignore
    • add yarn.lock (as it proved to be faster/safer)

[lidel]

🚲🏠 ipfs-companion-bundle.js (or just bundle.js) may be a better name (common.js is too vague, we've just deprecated it in master branch)

[alanshaw]

How about ipfs-companion-common.js? It's not the full bundle, just the dependencies that are common to more than one of our bundles.

[lidel]

Sounds good!

ps. This PR is our priority, as it gives us the biggest bang for the buck (potentially closes #306) and is required by other PRs. Let me know when you feel it is ready to merge, I will do my best to find time to review it before anything else 👍

[alanshaw]

Fixing the tests is the only thing left to do for this...

[lidel]

Just a thought: if it makes things easier, feel free to remove code coverage from karma.conf.js (coveralls etc). We can add it at a later time.
Getting unit tests to work again will be enough to merge this.

[alanshaw]

Morning @lidel

I'm on getting the tests working, I'll get this finished today and will shout when it's ready! The init tests are tricky now 😝 so if I can't get these fixed I might have to skip them temporarily until we can get time for a lib/ipfs-companion.js refactor.

The yarn lock file will get us a re-producible build since it locks dependencies of this project and of this project's dependencies.

[alanshaw]

👋 @lidel

I had to do a fair bit more refactoring of lib/ipfs-companion.js than I anticipated so it took longer than I expected. On the plus side, the modules I've pulled out are easier to test and reason about so that's nice!

The tests are now run in node with mocha (I moved them into the functional folder so maybe that needs to be renamed 😐)...it was the easiest thing to do to get them to pass...we lose the syntax checking of running them in a Firefox browser, but I think that's about all. It's easy enough to attach a window, browser and URL object to Node's global for testing and eventually I'd quite like for the code to not assume any globals in the future.

I think it's worth investigating some actual integration tests, but I don't know if it's even possible to install and test a web extension with selenium or whatever!

Of note, I've changed the npm test script to only run the functional tests...there are no tests in npm run test:unit anymore anyway.

I've also had to .skip the onStorageChange test - this is currently hanging and I haven't had time to figure out why (although the test does pass!).

Let me know if you have any questions/comments! Sorry this PR has grown so big!

[olizilla]

Of note, if i running the docker-build target from #313 shows that there is a 2 line diff in the common bundle, coming from a package.json!

$ diff -r --brief build/ipfs_companion-2.0.15 build-docker/ipfs_companion-2.0.15
Files build/ipfs_companion-2.0.15/dist/ipfs-companion-common.js and build-docker/ipfs_companion-2.0.15/dist/ipfs-companion-common.js differ

$ diff build/ipfs_companion-2.0.15/dist/ipfs-companion-common.js build-docker/ipfs_companion-2.0.15/dist/ipfs-companion-common.js 
63182c63182
<       "/Users/oli/Code/ipfs/ipfs-companion"
---
>       "/src"
63206c63206
<   "_where": "/Users/oli/Code/ipfs/ipfs-companion",
---
>   "_where": "/src",

Which is dull. This is from an empty node_modules done via docker and via local build. All other files in the dist are identical.

The issue is caused by ipfs-api require-ing it's own package.json. That should be fine, but when we npm install npm adds _prefixed keys to dependencies package.json, which seem to contain some info specific to the current install path:

exports = module.exports = () => {
  return {
    'api-path': '/api/v0/',
    'user-agent': `/node-${pkg.name}/${pkg.version}/`,
    host: 'localhost',
    port: '5001',
    protocol: 'http'
  }
}

},{"../../package.json":92}],92:[function(require,module,exports){
module.exports={
  "_args": [
    [
      "ipfs-api@15.0.1",
      "/src"
    ]
  ],
  "_from": "ipfs-api@15.0.1",
  "_id": "ipfs-api@15.0.1",
  "_inBundle": false,
  "_integrity": "sha512-rRh0bbKNVcHLbIKNeB72P6uVSbzMB9uGkD8t5DuTo3MftRR5WmSLacaY/Dunu1H03oGcZms4dWNKDLGhqidteQ==",
  "_location": "/ipfs-api",
  "_phantomChildren": {},
  "_requested": {
    "type": "version",
    "registry": true,
    "raw": "ipfs-api@15.0.1",
    "name": "ipfs-api",
    "escapedName": "ipfs-api",
    "rawSpec": "15.0.1",
    "saveSpec": null,
    "fetchSpec": "15.0.1"
  },
  "_requiredBy": [
    "/"
  ],
  "_resolved": "https://registry.npmjs.org/ipfs-api/-/ipfs-api-15.0.1.tgz",
  "_spec": "15.0.1",
  "_where": "/src",
  "author": {

see the line _where key in that screed.

That's browserified version of this: https://github.com/ipfs/js-ipfs-api/blob/2970c9f81efef28b55aba0ba0be40d7f604c1ec6/src/utils/default-config.js

I think we can fix it. I'll take a look when I get home. I'd be happy for this to get merged and that fixed outside of this PR if I don't get there first. Failing that, i think give that moz will be diffing things, and there goal is to find extensions that are trying to hide malware, I think we might be ok to submit it as is, with an explanation, as the diff would show it's harmless. Not ideal, I know.

[lidel]

@alanshaw Thank you! LGTM, I will merge it after a quick smoke-test.

I think running non-integration tests in node is fine ("non-integration tests" == everything that does not interact with real browser object, but uses sinon-chrome or a global stub). I raised the need for tooling for running tests against real browser in mozilla/web-ext#5 but it was not addressed yet.
I am keeping an open ticket for creating integration tests (#165) in case we want to implement it before an upstream solution is presented, but not everything can be tested without proper APIs in the browser / web-ext.
As for "unit vs functional" terminology, let's drop the use of "unit" and stick with "functional". We will have functional ones that run in node and at some point in future "integration" ones (maybe even per browser).

@olizilla I agree, let's submit it as is and wait for feedback from Mozilla. If you come up with a solution to the diff problem, hit me up with separate PR 👍

[lidel]

FYI I created a test release v2.1.0rc1 with these changes.
My plan is to smoke-test until Mozilla blocks v2.0.15, which will force us to release v2.1.0 🙃