simplify npm release process #4479

derhuerst · 2017-02-08T13:09:22Z

When trying to use @parity/parity.js, I noticed a lot of significant shortcomings of our npm-published libraries as well as some minor problems. This PR intends to create a starting point to improve the shape of these libraries.

I tried to make the build & publish process more transparent. Instead of copying files using the Webpack copy plugin into a temp directory, I now does a plain cp -r src/something npm/library-name/src and then transpiles these files using plain Babel (see npm run scripts/dryrun-npm.sh).

This means that we publish ES5 source code as individual files instead of a bundle including all dependencies & polyfills. There's a few advantages involved:

consuming @parity/parity.js, you can now see what's going on in a call to the library, e.g. if you passed invalid parameters and the error messages are not 100% helpful
stack traces, including source maps should work
having the bare transpiled source code, the bundler of my consuming project can decide wether to bundle for browsers or for node, and which libraries to bundle or to exclude.

Because this PR deprecates the .npmjs temp directory, it would make sense if we handle npm/library-name as the canonical root directory of our npm-published libraries.

just like before, we need to make sure all dependencies are properly declared in its package.json, especially as we'll (probably) remove more polyfills & tooling from the published library (see https://github.com/ethcore/parity/pull/4448#issuecomment-277748842)
we pickup the version of our root package.json, put it into the library package.json and commit these changes. this makes it transparent which library will be published and gives us a better chance to move to a server-friendly versioning scheme in the future.

I also changed the smoke tests inside npm/library-name/test to work with the core assert module. This has the advantage that the tests can live where the library gets copied to and we don't need to install additional dependencies in a subfolder.

Moving on from this PR, I see the following improvements to be made (but this is IMO):

remove all polyfills from the libraries and state in their readme that these are required. this prevents duplicate code of consuming users and enables them to choose how & where to put these dependencies in place. this is how the majority of the well-known maintained libraries on NPM does it.
we might fully move the code into npm/library-name. this has several advantages:
- stability: we don't randomly publish new libraries whenever a UI build succeeds
- testability: independent from the large UI codebase, with a simpler setup
- discoverability by 3rd parts users: people will be able to find & report bugs & shortcomings, as they may feel overwhelmed by the size of src.

Things still to be done:

remove ~ from src/api and src/abi. it's only a few occurrences that don't seem to be worth the build setup.
test scripts/release.sh and npm run clean more. i'm sure i missed things
figure out where to put the E2E tests in test/e2e. there's also test/npm*.js that seem partially obsolete. – 0465034
solve the issue with requiring/importing parity.js twice (it is impossible right now) – de56706
deal with whatwg-fetch -> isomorphic-fetch #4448 – de56706
npm/etherscan depends on api/util

Tradeoffs made in this PR:

As of de56706, we expect the users consuming our libraries to bundle & calls the required poly fills by themselves.
ffd960c duplicates abi/util/address into 3rdparty/etherscan/util in order to make @parity/etherscan completely independent from @parity/parity.js. A more elegant solution would be to dynamically include the required file, but this seems hard to do without a Webpack-like setup. (We dropped the Webpack setup because it lacks transparency and is very complicated).

ngotchac

Several issues:

Some reference to the api are in the other packages (eg. in etherscan/accounts). This should not occur, or parity.js should be a dependency and used as one.
The index files of src and dist reference the 3rd party folder. This doesn't exist in the npm folder context

Why not having one script for the dryrun and not dryrun version of the npm scripts, one with one extra-line for publishing ? Right now one of them has some issues, so it would prevent that and having to maintain two scripts

ngotchac · 2017-02-08T14:07:17Z

js/scripts/dryrun-npm.sh


- cd $DIRECTORY
+pushd .; cd npm/jsonrpc


etherscan

ngotchac · 2017-02-08T14:07:32Z

js/scripts/dryrun-npm.sh


-done
-cd ..
+pushd .; cd npm/jsonrpc


shapeshift

ngotchac · 2017-02-08T14:07:49Z

js/scripts/dryrun-npm.sh

+cp -r src/api npm/parity/src/api
+env LIBRARY=parity npm run ci:build:npm
+
+pushd .; cd npm/jsonrpc


cd npm/parity

Changing from `~` to `../..` is a step backwards in terms of dev UX. Nevertheless, it seems worth it, because we can get rid of additional plugins/bundlers when publishing the libraries to npm. See #4479.

derhuerst · 2017-02-08T14:36:12Z

Note: We need to properly test scripts/release.sh before this gets merged, as we have automatic publishing to npm.

derhuerst · 2017-02-08T15:39:31Z

Why not having one script for the dryrun and not dryrun version of the npm scripts, one with one extra-line for publishing ? Right now one of them has some issues, so it would prevent that and having to maintain two scripts

@ngotchac I Did this in 5500e21.

Even though this commit introduces duplicated code, it seems worth it, because we can get rid of additional plugins/bundlers when publishing the libraries to npm. See #4479 for more details.

ngotchac · 2017-02-08T22:17:36Z

I updated a bit the PR. Everything seems to work fine, but we might want to test every possibility before merging. Might also want to update as a Major release, since we got rid of the babel + promise polyfills

jacogr · 2017-02-09T06:47:18Z

Don't do this -

ffd960c duplicates abi/util/address into 3rdparty/etherscan/util

We cannot maintain stuff (untested) in 2 places.

jacogr · 2017-02-09T15:50:43Z

js/test/npmJsonRpc.js


 if (typeof JsonRpc !== 'object') {
 throw new Error('JsonRpc');
 }

- console.log(JsonRpc);
+ console.log('JSON RPC Endpoints:', Object.keys(JsonRpc));


Think the original was actually a debug that slipped through, however this makes (slightly) more sense.

jacogr · 2017-02-09T15:50:46Z

js/webpack/libraries.js

@@ -30,7 +30,7 @@ module.exports = {
 // library
 'inject': ['./inject.js'],
 'web3': ['./web3.js'],
- 'parity': ['./parity.js']
+ 'parity': ['./library.parity.js']


We have things mixed up here. This version of parity is supposed to be the version served via /parity-utils/{inject,parity}.js - library refers to the npm version.

Fully agree that it's not a for the library. @ngotchac and I discussed renaming it to dev.parity.js. What do you think?

It is not dev.parity either :)

It is actually the version of parity.js that gets used by all dapps running inside Parity. e.g. https://github.com/gavofyork/gavcoin/blob/master/index.html#L11

If I don't confuse things, we can move src/parity.js to npm/parity/src/index.js and rename src/library.parity.js to src/parity.js.

Yes, the changes here

https://github.com/ethcore/parity/pull/4479/files#diff-5a5d605811790e35c3e21f577912e89eR17

and here

https://github.com/ethcore/parity/pull/4479/files#diff-161120f1d7582520482df9363dee21acR17

Are the ones where you went wrong. You tried to convert the one into the other, instead of understanding the purpose. (Changes basically copies the one to the other - more or less) Should have kept it, moved library.parity.js to index.js (like done elsewhere) and have been done, but rather these changes got them mixed.

5da69d7 renames parity.js to parity.npm.js and reverts library.parity.js to parity.js.

ngotchac · 2017-02-13T13:49:07Z

js/src/api/util/address.js

@@ -0,0 +1 @@
+../../abi/util/address.js


Why not moving the file in api ?

@ngotchac Maybe i missed something, but then abi would depend on api, which we try to prevent.

Not sure I fully followed the discussion about what goes in ABI vs. API. However it just looks weird to have a symlink to symlink.

~/abi/util is not part of the exposed interfaces (i.e. stuff on Abi.*), so don't use it. :)

Has anybody tested the symlink stuff on Windows? (Anything needs to work across Mac, Linux & Windows - although we may get away here since publishing is only done from Linux)

And I do agree, symlink to symlink is (very) weird, but (slightly) better than the alternative at least.

jacogr · 2017-02-16T10:29:46Z

Moving from grumle to got issues.

First the grumble part -

We are still duplicating the LICENCE files - copy them in as per original.
We are still doing the redirects
We are still not complying with code-base wide standards for testing

Secondly the has-issues part -

Symlinks are a no-no on Windows. Possible suggestions (first thing that pops to mind)
- pass an Api class/instance to etherscan so it can use {Api,api}.util.* - not perfect, but since we expect users to be in our ecosystem and this one is not bound to get millions of users, a trade-off.
- Copy the file in when bundling (distant second)
We are not dealing sufficiently well with the polyfill issue here. This PR brings back the first user-reported issue after the 1.4 release, the dreaded "Array.includes is not a function". As library providers, we can expect dep libraries to be there, polyfills are a slippery slope. Suggestions (one or multiples) -
- No ES2015/2016 type constructs allowed in the 3rdparty stuff (? refactoring where applicable)
- Detect and include polyfills where applicable (e.g. no Array.includes, require('es7-shim'))
- We need to have something that deals with fetch. Users should just include the library without having to polyfill anything and it works. (As it did with the webpack-compiled version - which initially had exactly the same issues as reported here)

derhuerst · 2017-02-16T15:36:19Z

First the grumble part -

We are still duplicating the LICENCE files - copy them in as per original.

We are still doing the redirects

We are still not complying with code-base wide standards for testing

Ok.

Symlinks are a no-no on Windows. Possible suggestions (first thing that pops to mind)

pass an Api class/instance to etherscan so it can use {Api,api}.util.* - not perfect, but since we expect users to be in our ecosystem and this > one is not bound to get millions of users, a trade-off.

The idea behind this PR is to get clean, decoupled libraries published to NPM. What you suggest would move us in the opposite direction.

Maybe that sounds too harsh, but arguing that only because people use our Ethererum node we can force them to use our other stuff seems to be awfully close to embrace extend extinguish. Arguing that the lib doesn't need to have a good quality (in a PR that tries to improve it) because it's never going to be often-used also seems weird.

Symlinks are a no-no on Windows. Possible suggestions (first thing that pops to mind)
[…]

Copy the file in when bundling (distant second)

Will do that as a trade-off.

We are not dealing sufficiently well with the polyfill issue here. This PR brings back the first user-reported issue after the 1.4 release, the dreaded "Array.includes is not a function". As library providers, we can expect dep libraries to be there, polyfills are a slippery slope.

Polyfills are basically shared globals, requireing them is in many cases considered bad practice, especially if they're environment-specific. requireing babel-polyfill breaks any codebase that includes it somewhere else.

Regarding Array.prototype.includes: Shouldn't babel transpile that to Array.prototype.indexOf?

We need to have something that deals with fetch. Users should just include the library without having to polyfill anything and it works.

Strongly disagree. It is neither very hard nor uncommon to have users to require/import polyfills before using a library. In our case it would look like this.

(As it did with the webpack-compiled version - which initially had exactly the same issues as reported here)

As stated, the Webpack version doesn't work in codebases that require babel-polyfill somewhere else. It effectively makes it impossible to use parity.js with ES2015+ code. It contains the wrong fetch polyfill if run in non-browser environments.

jacogr · 2017-02-16T18:11:43Z

Not sure what Babel does (EDIT: babel-runtime), Array.includes always creates an issue, hence using it as an example. (Which is actually very real, first support ticket on parity.js)

And sorry, but I'm not going to support anything that is going to create yet more overhead in "this doesn't work". We need to find an elegant polyfill solution if the current is not it. (And I'm probably leaning towards ignoring fetch here since that may be an unsolvable issue.)

It is always very easy to throw out the baby with the bathwater, but you need to understand why the baby got dirty in the first place. Dependent libraries are one thing, having to deal with other stuff lands us in the category of "this is too much effort". Steps should be -

npm install
it just works

And no, we are not at #2.

jacogr · 2017-02-16T18:13:32Z

js/npm/etherscan/package.json

+ "peerDependencies": {
+ "es6-promise": "^4.0.5",
+ "isomorphic-fetch": "^2.2.1",
+ "babel-polyfill": "^6.22.0"


Wondering out loud... Could https://github.com/es-shims not (maybe, just maybe) be a more elegant (can include it without issues) solution. Worth a go, e.g.

require('es6-shim'); require('es7-shim'); ...stuff goes here (without issues)...

Scratch that.

Babel actually has the solution - the transform runtime plugin. Basically means only babel-runtime is added as a package dep, and the runtime includes the polyfill without polluting the global namespace. No Promise, polyfill and other include-at-head crainess. (Well, according to them - this is their library mode)

jacogr · 2017-02-16T18:15:25Z

js/src/3rdparty/etherscan/util/address.js

@@ -0,0 +1 @@
+../../../api/util/address.js


This should probably be removed now?

jacogr · 2017-02-16T18:15:51Z

js/scripts/prepare-npm-libraries.sh

+printf "\n***** Building jsonrpc for NPM ********"
+printf "\n***************************************\n\n"
+npm run ci:build:jsonrpc
+cp LICENSE npm/jsonrpc/LICENSE


jacogr · 2017-02-16T18:16:32Z

js/.gitignore

+
+npm/*/src
+npm/*/dist
+npm/jsonrpc/index.json


Shouldn't the LICENSE files be here now as well since it is copied into tracked paths after the fact?

jacogr · 2017-02-16T19:09:51Z

js/npm/etherscan/README.md

+```js
+require('babel-polyfill');
+require('es6-promise').polyfill();
+require('isomorphic-fetch');


Really ridiculously complicated just to get started. Not an option. Been there, done that, got the scars.

(As indicated, we may get away with fetch, we could by indicating that you need to include it for it in a Node environment and what to do for browsers - Edge, FF, Chrome has it, others lag.)

For fetch - could... maybe... use the package.json browser field (all modern bundler support this), and for the node entry include isometric-fetch for the browser one whatwg-fetch.

(Together with babel-runtime it could get us to a "just works" point.)

For fetch - could... maybe... use the package.json browser field (all modern bundler support this), and for the node entry include isometric-fetch for the browser one whatwg-fetch.

That is what isomorphic-fetch does.. But people consuming the library want to be able to bundle it themselves. That's why it's in the readme.

gavofyork · 2017-03-23T12:26:06Z

@jacogr can we do something with this?

jacogr · 2017-04-02T09:29:08Z

Closing. Re-open PR when actually being worked on.

derhuerst added 8 commits February 7, 2017 19:36

move src/library.*.js -> npm/*/src/index.js

e1147dc

simplify scripts/build-rpc-json.js

76dc277

simplify scripts/dryrun-npm.sh & scripts/release.sh

9e298b0

basic smoke tests for modules

e2f4bff

bugfixes 🐛

2ee8948

run module tests on release

b62e3d9

cleanup, bugfix 🐛

68d5917

modules: jsnext:main field, LICENSE file

4c6f20d

ngotchac suggested changes Feb 8, 2017

View reviewed changes

ngotchac added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. M6-rpcapi 📣 RPC API. labels Feb 8, 2017

fix npm script, use relative paths

d781e0e

Changing from `~` to `../..` is a step backwards in terms of dev UX. Nevertheless, it seems worth it, because we can get rid of additional plugins/bundlers when publishing the libraries to npm. See #4479.

derhuerst added 3 commits February 8, 2017 16:13

fix copy scripts

ef7081f

fix library entry points

bb98d5b

dryrun-npm -> prepare-npm-libraries, reuse it in release.sh

5500e21

copy abi/util/address -> 3rdparty/etherscan/util

ffd960c

Even though this commit introduces duplicated code, it seems worth it, because we can get rid of additional plugins/bundlers when publishing the libraries to npm. See #4479 for more details.

derhuerst mentioned this pull request Feb 8, 2017

whatwg-fetch -> isomorphic-fetch #4448

Closed

derhuerst and others added 4 commits February 8, 2017 18:04

polyfills as peer dependencies

de56706

add dependencies, update readmes

5c63681

Add mkdir -p to make sure the directories exist

d648f82

Fixed tests // Fixed published package.json

4173f1b

ngotchac approved these changes Feb 8, 2017

View reviewed changes

ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 8, 2017

jacogr added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 9, 2017

jacogr reviewed Feb 9, 2017

View reviewed changes

jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 9, 2017

derhuerst added 2 commits February 13, 2017 14:13

parity.js -> parity.npm.js, library.parity.js -> parity.js

5da69d7

etherscan: import address util from api

4b6657d

ngotchac reviewed Feb 13, 2017

View reviewed changes

Merge branch 'master' into jr-simplify-release

fb1cdc1

jacogr added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 16, 2017

derhuerst added 4 commits February 16, 2017 16:42

copy license file to libs

3574107

build jsonrpc without redirect

9a478f0

move lib tests to test/e2e

0465034

use mocha for lib tests

a82fdbc

jacogr reviewed Feb 16, 2017

View reviewed changes

derhuerst added 4 commits February 21, 2017 14:39

merge master into jr-simplify-release

7642710

npm libs: babel-polyfill -> babel-runtime

d55d051

npm libs: update docs, ignore LICENSE files

5c9a018

npm libs: babel-runtime as non-peer dependency

d8e282e

jacogr closed this Apr 2, 2017

ngotchac deleted the jr-simplify-release branch September 25, 2017 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify npm release process #4479

simplify npm release process #4479

derhuerst commented Feb 8, 2017 •

edited

Loading

ngotchac left a comment

ngotchac Feb 8, 2017

ngotchac Feb 8, 2017

ngotchac Feb 8, 2017

derhuerst commented Feb 8, 2017

derhuerst commented Feb 8, 2017 •

edited

Loading

ngotchac commented Feb 8, 2017

jacogr commented Feb 9, 2017

jacogr Feb 9, 2017

jacogr Feb 9, 2017

derhuerst Feb 9, 2017

jacogr Feb 9, 2017

derhuerst Feb 9, 2017

jacogr Feb 10, 2017

derhuerst Feb 13, 2017

ngotchac Feb 13, 2017

derhuerst Feb 13, 2017

ngotchac Feb 13, 2017

jacogr Feb 15, 2017 •

edited

Loading

jacogr commented Feb 16, 2017 •

edited

Loading

derhuerst commented Feb 16, 2017

jacogr commented Feb 16, 2017 •

edited

Loading

jacogr Feb 16, 2017 •

edited

Loading

jacogr Feb 16, 2017

jacogr Feb 16, 2017

jacogr Feb 16, 2017

jacogr Feb 16, 2017 •

edited

Loading

jacogr Feb 16, 2017 •

edited

Loading

jacogr Feb 16, 2017

derhuerst Feb 21, 2017 •

edited

Loading

gavofyork commented Mar 23, 2017

jacogr commented Apr 2, 2017

		@@ -0,0 +1 @@
		../../abi/util/address.js

		@@ -0,0 +1 @@
		../../../api/util/address.js

simplify npm release process #4479

simplify npm release process #4479

Conversation

derhuerst commented Feb 8, 2017 • edited Loading

ngotchac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derhuerst commented Feb 8, 2017

derhuerst commented Feb 8, 2017 • edited Loading

ngotchac commented Feb 8, 2017

jacogr commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

jacogr commented Feb 16, 2017 • edited Loading

derhuerst commented Feb 16, 2017

jacogr commented Feb 16, 2017 • edited Loading

jacogr Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

jacogr Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derhuerst Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

gavofyork commented Mar 23, 2017

jacogr commented Apr 2, 2017

derhuerst commented Feb 8, 2017 •

edited

Loading

derhuerst commented Feb 8, 2017 •

edited

Loading

jacogr Feb 15, 2017 •

edited

Loading

jacogr commented Feb 16, 2017 •

edited

Loading

jacogr commented Feb 16, 2017 •

edited

Loading

jacogr Feb 16, 2017 •

edited

Loading

jacogr Feb 16, 2017 •

edited

Loading

jacogr Feb 16, 2017 •

edited

Loading

derhuerst Feb 21, 2017 •

edited

Loading