build: introduce a 'prepare' script in embark's package.json #1128

michaelsbradleyjr · 2018-11-25T17:38:41Z

build: introduce a prepare script in embark's package.json

TL;DR

These changes affect workflow with yarn. To prevent embark's prepare script from running undesirably:

If node_modules is in place and you're reinstalling after switching branches:

yarn run install_all

If node_modules is missing (fresh clone or deleted):

EMBARK_NO_PREPARE=t yarn install && yarn run install_all

It's not recommended to set EMBARK_NO_PREPARE in your environment (e.g. in .bashrc) since that would interfere with embark's release script if/when you run it.

1. Specify embark's build-related steps in the prepare script of package.json.

When embark is installed directly from GitHub the prepare script results in a "pre install" phase (handled automatically by npm/yarn) that fetches devDependencies, builds embark (including embark-ui), packs a tarball with the same steps (minus testing and tree-checking) as would happen during an embark release, and finally does a production install from that tarball.

Important point: installs from GitHub must be performed with yarn; they're no longer possible with npm since during the "pre install" phase npm will honor embark's .npmrc and "engines" settings.

The following will work correctly after this commit is merged:

yarn [global] add git+https://github.com/embark-framework/embark.git

Use of "hosted git" shortcuts (e.g. embark-framework/embark#bracnh) won't work correctly because yarn doesn't fully support them. See: yarnpkg/yarn#5235.

It's important to use git+https urls. Following a succesful install with git+https it is possible to use a "hosted git" shortcut or https url, but that's owing to a subtle and unreliable interaction between yarn's cache and yarn's logic for installing from a url/shortcut.

2. Adjust the npm configs (.npmrc) for embark/-ui so that yarn run [cmd] [--opt] can be used in place of npm run [cmd] -- [--opt].

Either way is okay for running scripts, they're equivalent, but note the requirement to use -- before specifying command options with npm run.

3. Introduce yarn configs (.yarnrc) for embark/-ui and include the check-files directive.

H/t to @alaibe for the recommendation.

4. Ignore embark's dist/typings and scripts directories when packing a tarball.

5. Refactor embark/-ui's npm-scripts in relation to the prepare script, and make other small improvements.

Notably, if the environment variable EMBARK_NO_PREPARE is truthy (from JS perspective) then embark's prepare script will exit early. This prevents install_all and prepare from getting stuck in a loop (install:core uses cross-env to set EMBARK_NO_PREPARE) and provides a mechanism for users to skip the prepare script when doing a fresh install:

EMBARK_NO_PREPARE=t yarn install

6. Give .js extensions to node scripts in embark's scripts/, remove the shebang lines, and have npm-scripts explicitly invoke them with node.

This arrangement works for all platforms: Linux, macOS, and Windows.

7. Adjust travis and appveyor configs.

Since at present there aren't any tests or other CI steps that make use of embark-ui's production build, set EMBARK_NO_PREPARE in the CI environments and invoke build:node directly.

Check the working tree after yarn install for embark/-ui. This detects situations where changes should have been committed to yarn.lock but were not. Check the working tree again at the end to detect situations where ignore files should have been adjusted but were not. Both checks could also detect other surprising behavior that needs to be investigated. Any time the working tree is not clean (there are untracked files or changes) CI will fail.

Drop CI runs for node 8.11.3 because that version ships with an older npm that results in unstaged changes to the test apps' package-lock.json files, causing the working tree check to fail at the end of the CI run. A simple workaround isn't apparent, but the matter can be revisited.

8. Refactor embark's release script in light of the prepare script.

Notably, do the push step only after npm publish completes successfully. This allows embark's prepare and prepublishOnly scripts to detect problems before a commit and tag are pushed to GitHub, avoiding a need to rebase/revert the remote release branch; the local branch will still need to have a commit dropped and tag deleted before rerunning the release script.

Prompt the user if the release script is not being run in --dry-run mode.

Provide additional visual indicators of --dry-run mode.

Force the user to supply --repo-branch [branch] if the intention is to release from a branch other than master.

michaelsbradleyjr · 2018-11-25T17:50:51Z

scripts/release.js has been manually tested very carefully to check that all steps behave as intended in both dry and non-dry runs for every success and error condition.

0x-r4bbit

Left some minor comments there.

I think it'd be a good time to introduce some "developer" docs for us where we can read about how to get Embark up and running an make releases etc.

0x-r4bbit · 2018-11-26T07:44:59Z

scripts/release.js

+      throw new Error();
+    }
+
+    logSuccess(`Current branch and relase branch are the same.`);


relase => release

Good catch, fixed! (and rebased with master)

I'll squash the post-feedback commits when it's time to merge into master.

0x-r4bbit · 2018-11-26T07:48:08Z

scripts/release.js

+      `npm publish`,
+      leftPad1Maybe(`${distTag ? `--tag ${distTag}` : ''}`),
+      leftPad1Maybe(`${dryRun ? '--dry-run' : ''}`)
+    ].join('');


Don't we wanna join with a space here? Otherwise we'd end up with npm publish--tag foo--dry-run no?

Ah nevermind, there's a leftPad1Maybe. Still, wouldn't it be simpler to remove that and just join with ' ' instead?

We could, but it would lead to extra spaces in the command string when distTag or dryRun is empty — not the end of the world but I'd prefer to not have extraneous spaces.

The idea with leftPad1/Maybe was to avoid a situation where someone in the future working on the script removes a space from a string because it looks like it was a typo and then, because testing the script is mostly a manual process, the mistake isn't realized until later. A function call is less likely to be accidentally removed, I think.

alaibe · 2018-11-26T09:36:19Z

.npmignore

@@ -9,10 +9,12 @@ CONTRIBUTING.md
 appveyor.yml
 babel.config.js
 dist/test
+dist/typings


Why are we not publishing the types?
If we want to produce embark as a lib, the types are helpful.

We can. At present, the typings scrips are being compiled into .js and effectively erased. We could:

Have babel ignore that directory in which case it will copy the .ts file/s without transpiling them.

Or continue excluding that directory in .npmignore but whitelist src/typcings/.

Preference?

I think we'd also need a "types" property in package.json. See: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

The "types" property is supposed to point to a "bundled declaration file". What's the right way to set that up?

That is a good question, usually TS generate those but I am not sure it is possible with babel, something to investigate.

I would say ignore my comments until we found a way to generate .d.ts file

There is an option in ts config:
"declaration": true, but because we don't emit the files this is not possible.

Something to investigate but unsure would be https://www.npmjs.com/package/dts-bundle

Maybe it's possible to emit only the bundled declaration. I can investigate, but I think making that change to the build should land in another PR.

Probably a good idea to tackle in a new PR, though let's not forget about it :)

michaelsbradleyjr · 2018-11-29T00:24:24Z

Several changes have been made in the latest WIP commit to deal with package resolution problems. I think it's probably okay that they go in this PR; this branch certainly provided a good context for testing their effects when embark is installed from GitHub with yarn, and when running embark in a development setup.

If @embark-framework/core-devs can take another look and it's judged okay, I'll squash into one commit and remove the changes needed label.

michaelsbradleyjr · 2018-11-29T00:52:17Z

src/lib/modules/whisper/js/communicationFunctions.js

@@ -93,8 +93,7 @@ function listenTo(options, callback) {
        return callback(null, data);
      }
      promise.cb(payload, data, result);
-    })
-    .catch(callback);


After making all of the web3 beta.34 modules direct dependencies of embark, the object returned by subscribe(...).on(...) doesn't have a .catch method. With the invocation removed, everything seems to be working okay in embark_demo.

michaelsbradleyjr · 2018-12-02T19:29:16Z

Module resolution commits were removed and a new PR was created for those: #1152

@alaibe

**TL;DR** These changes affect workflow with yarn. To prevent embark's `prepare` script from running undesirably: - If node_modules is in place and you're reinstalling after switching branches: ``` yarn run install_all ``` - If node_modules is missing (fresh clone or deleted): ``` EMBARK_NO_PREPARE=t yarn install && yarn run install_all ``` It's not recommended to set `EMBARK_NO_PREPARE` in your environment (e.g. in `.bashrc`) since that would interfere with embark's `release` script if/when you run it. ----------------- **1.** Specify embark's build-related steps in the `prepare` script of package.json. When embark is installed directly from GitHub the `prepare` script results in a "pre install" phase (handled automatically by npm/yarn) that fetches devDependencies, builds embark (including embark-ui), packs a tarball with the same steps (minus testing and tree-checking) as would happen during an embark release, and finally does a production install from that tarball. Important point: installs from GitHub must be performed with yarn; they're no longer possible with npm since during the "pre install" phase npm will honor embark's `.npmrc` and `"engines"` settings. The following will work correctly after this commit is merged: ``` yarn [global] add git+https://github.com/embark-framework/embark.git ``` Use of "hosted git" shortcuts (e.g. `embark-framework/embark#bracnh`) won't work correctly because yarn doesn't fully support them. See: yarnpkg/yarn#5235. It's important to use `git+https` urls. Following a succesful install with `git+https` it is possible to use a "hosted git" shortcut or `https` url, but that's owing to a subtle and unreliable interaction between yarn's cache and yarn's logic for installing from a url/shortcut. **2.** Adjust the npm configs (`.npmrc`) for embark/-ui so that `yarn run [cmd] [--opt]` can be used in place of `npm run [cmd] -- [--opt]`. Either way is okay for running scripts, they're equivalent, but note the requirement to use `--` before specifying command options with `npm run`. **3.** Introduce yarn configs (`.yarnrc`) for embark/-ui and include the `check-files` directive. H/t to @alaibe for the recommendation. **4.** Ignore embark's `dist/typings` and `scripts` directories when packing a tarball. **5.** Refactor embark/-ui's npm-scripts in relation to the `prepare` script, and make other small improvements. Notably, if the environment variable `EMBARK_NO_PREPARE` is truthy (from JS perspective) then embark's `prepare` script will exit early. This prevents `install_all` and `prepare` from getting stuck in a loop (`install:core` uses cross-env to set `EMBARK_NO_PREPARE`) and provides a mechanism for users to skip the `prepare` script when doing a fresh install: ``` EMBARK_NO_PREPARE=t yarn install ``` **6.** Give `.js` extensions to node scripts in embark's `scripts/`, remove the shebang lines, and have npm-scripts explicitly invoke them with node. This arrangement works for all platforms: Linux, macOS, and Windows. **7.** Adjust travis and appveyor configs. Since at present there aren't any tests or other CI steps that make use of embark-ui's production build, set `EMBARK_NO_PREPARE` in the CI environments and invoke `build:node` directly. Check the working tree after `yarn install` for embark/-ui. This detects situations where changes should have been committed to `yarn.lock` but were not. Check the working tree again at the end to detect situations where ignore files should have been adjusted but were not. Both checks could also detect other surprising behavior that needs to be investigated. Any time the working tree is not clean (there are untracked files or changes) CI will fail. Drop CI runs for node 8.11.3 because that version ships with an older npm that results in unstaged changes to the test apps' `package-lock.json` files, causing the working tree check to fail at the end of the CI run. A simple workaround isn't apparent, but the matter can be revisited. **8.** Refactor embark's `release` script in light of the `prepare` script. Notably, do the push step only after `npm publish` completes successfully. This allows embark's `prepare` and `prepublishOnly` scripts to detect problems before a commit and tag are pushed to GitHub, avoiding a need to rebase/revert the remote release branch; the local branch will still need to have a commit dropped and tag deleted before rerunning the `release` script. Prompt the user if the `release` script is not being run in `--dry-run` mode. Provide additional visual indicators of `--dry-run` mode. Force the user to supply `--repo-branch [branch]` if the intention is to release from a branch other than `master`.

michaelsbradleyjr requested a review from a team November 25, 2018 17:38

0x-r4bbit reviewed Nov 26, 2018

View reviewed changes

alaibe approved these changes Nov 26, 2018

View reviewed changes

michaelsbradleyjr force-pushed the build/prepare-script branch from 3af2618 to 1300466 Compare November 26, 2018 14:38

jrainville approved these changes Nov 26, 2018

View reviewed changes

michaelsbradleyjr force-pushed the build/prepare-script branch 2 times, most recently from 6862f97 to d7e48a4 Compare November 26, 2018 18:54

michaelsbradleyjr added the changes needed label Nov 26, 2018

michaelsbradleyjr force-pushed the build/prepare-script branch 2 times, most recently from 4afa086 to 38f31e0 Compare November 28, 2018 23:33

michaelsbradleyjr commented Nov 29, 2018

View reviewed changes

michaelsbradleyjr force-pushed the build/prepare-script branch 2 times, most recently from e7b72d8 to ca55834 Compare December 1, 2018 22:24

michaelsbradleyjr removed the changes needed label Dec 2, 2018

emizzle approved these changes Dec 2, 2018

View reviewed changes

michaelsbradleyjr force-pushed the build/prepare-script branch from ca55834 to 4656235 Compare December 3, 2018 15:25

michaelsbradleyjr merged commit 73d5a72 into master Dec 3, 2018

michaelsbradleyjr mentioned this pull request Dec 4, 2018

fix(@embark/core): fix to allow large ether values #1142

Merged

0x-r4bbit deleted the build/prepare-script branch December 4, 2018 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: introduce a 'prepare' script in embark's package.json #1128

build: introduce a 'prepare' script in embark's package.json #1128

michaelsbradleyjr commented Nov 25, 2018 •

edited

Loading

michaelsbradleyjr commented Nov 25, 2018

0x-r4bbit left a comment

0x-r4bbit Nov 26, 2018

michaelsbradleyjr Nov 26, 2018

michaelsbradleyjr Nov 26, 2018

michaelsbradleyjr Nov 26, 2018

0x-r4bbit Nov 26, 2018

0x-r4bbit Nov 26, 2018

michaelsbradleyjr Nov 26, 2018

alaibe Nov 26, 2018

michaelsbradleyjr Nov 26, 2018 •

edited

Loading

alaibe Nov 26, 2018

alaibe Nov 26, 2018

michaelsbradleyjr Nov 26, 2018

emizzle Dec 2, 2018

michaelsbradleyjr commented Nov 29, 2018 •

edited

Loading

michaelsbradleyjr Nov 29, 2018

michaelsbradleyjr commented Dec 2, 2018

build: introduce a 'prepare' script in embark's package.json #1128

build: introduce a 'prepare' script in embark's package.json #1128

Conversation

michaelsbradleyjr commented Nov 25, 2018 • edited Loading

michaelsbradleyjr commented Nov 25, 2018

0x-r4bbit 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

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

michaelsbradleyjr Nov 26, 2018 • 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

Choose a reason for hiding this comment

michaelsbradleyjr commented Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

michaelsbradleyjr commented Dec 2, 2018

michaelsbradleyjr commented Nov 25, 2018 •

edited

Loading

michaelsbradleyjr Nov 26, 2018 •

edited

Loading

michaelsbradleyjr commented Nov 29, 2018 •

edited

Loading