Skip to content

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Jan 30, 2018

The current build has 'prepare' for each package to:

  1. Run tsc for dist
  2. Run tsc for dist6
  3. Run apidocs generation

Neither 2 or 3 is required for most of our development tasks. Both steps
are pretty time-consuming.

Please note prepare is triggered with npm install, which is also part
of erna bootstrap.

This move will:

  1. Speed up our scripts for most cases
  2. Continue to ensure dist/dist6 builds and apidocs before publish

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Related API Documentation was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

@raymondfeng
Copy link
Contributor Author

With this PR, an incremental npm run bootstrap only takes a few seconds:

real	0m9.828s
user	0m2.601s
sys	0m0.584s

A brand new npm run bootstrap costs less than 3 mins.

real	2m50.900s
user	1m6.066s
sys	0m18.303s

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Amazing performance improvements!! Great job! 👍

Just a question ... is it possible for apidocs to ever end up in a state where it may break? If so ... should we be testing that with each PR or not? (I'm ok to skip testing it as long as it won't lead to other issues down the road).

.travis.yml Outdated

script:
- npm run test:ci
- npm run test
Copy link
Contributor

Choose a reason for hiding this comment

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

script: and -npm run test can be removed in it's entirety as that's the default for travis. It was only added because we were running npm run test:ci.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Even though npm run test is the same as npm test, I prefer to use npm test.

appveyor.yml Outdated
- node --version
- npm --version
- npm run test:ci
- npm run test
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this originally used to be just npm test.

The current build has 'prepare' for each package to:

1. Run tsc for dist
2. Run tsc for dist6
3. Run apidocs generation

Neither 2 or 3 is required for most of our development tasks. Both steps
are pretty time-consuming.

Please note `prepare` is triggered with `npm install`, which is also part
of `erna bootstrap`.

This move will:
1. Speed up our scripts for most cases
2. Continue to ensure dist/dist6 builds and apidocs before publish
@raymondfeng
Copy link
Contributor Author

Just a question ... is it possible for apidocs to ever end up in a state where it may break? If so ... should we be testing that with each PR or not? (I'm ok to skip testing it as long as it won't lead to other issues down the road).

In most cases, apidocs should not break as long as tsc passes. The only situation I can imagine is that we have bugs in strong-docs that chokes on certain js/ts docs.

We can configure CI to run apidocs for master (not PR). The script is still there if we want to make sure before we commit. The PR will also ensure no bad things will be published to npm.

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

I'm ok to not test apidocs on CI personally unless we start running into issues down the road.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LTGM.

I wanted to comment on the fact that prepublishOnly is not available in Node.js 6.x, therefore publishing new releases while running Node.js 6.x will most likely produce broken packages (I didn't try to verify this from obvious reasons). However, prepare was added at the same time as prepublishOnly , therefore the change in this pull request is not making the current state any worse AFAICT.

@bajtos
Copy link
Member

bajtos commented Jan 30, 2018

CI failed on AppVeyor with a cryptic error (see the log), I have triggered a rebuild.

@bajtos bajtos merged commit afac0cf into master Jan 30, 2018
@bajtos bajtos deleted the optimize-build branch January 30, 2018 09:13
@bajtos
Copy link
Member

bajtos commented Jan 30, 2018

@raymondfeng I landed this patch myself so that I can follow the new style in the example projects I am bringing to the monorepo. Thank you for reducing the wait times we are encountering during development!

bajtos added a commit that referenced this pull request Jan 30, 2018
This reverts commit afac0cf
(pull request #928) as an experimental attempt to fix our failing build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants