Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(platform): Improve consistency (yarn), and async #84

Merged
merged 1 commit into from
Jul 16, 2017

Conversation

bfricka
Copy link
Contributor

@bfricka bfricka commented Jul 15, 2017

  • Add CircleCI override to use latest yarn for deterministic builds
  • Refactor some overly obtuse functions
  • Use async throughout
  • Add commitizen-friendly badge

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.563% when pulling 8be24cf on bfricka:dev/build-improvements-2 into 8804156 on ngrx:master.

@bfricka
Copy link
Contributor Author

bfricka commented Jul 15, 2017

Couple things about this PR:

  1. Using [email protected] because several earlier versions have a regression that causes issues symlinking to ./node_modules/.bin. See this issue.

  2. The biggest improvement is using yarn instead of npm. This improves perf a bit, but much more importantly, it gives build determinism.

  3. This doesn't improve build perf like the last one. The async stuff is just for consistency, and likely wouldn't have a demonstrable impact unless the numbers of blocking tasks got significantly higher. That said, it's probably a good idea for io stuff to be async anyways, if only because most of it already was anyways.

I was actually planning for this PR to be unit tests, but with the latest commits (lots of new deps), I ended up running into the aforementioned symlink bug with my version of yarn, and couldn't get the project to build. So I went down the rabbit hole, ended up tweaking a few things, and here we are.

- Add CircleCI override to use latest yarn for deterministic builds
- Refactor some overly obtuse functions
- Use async throughout
- Add commitizen-friendly badge
@bfricka bfricka force-pushed the dev/build-improvements-2 branch from 8be24cf to e403aca Compare July 15, 2017 23:16
@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.563% when pulling e403aca on bfricka:dev/build-improvements-2 into 8804156 on ngrx:master.

@bfricka
Copy link
Contributor Author

bfricka commented Jul 15, 2017

One more thing: We should probably update contributing, to refer to yarn. Until npm@5 becomes more mainstream, the only way for determinism is something like shrinkpack, but since this repo is already using yarn, it seems reasonable to consider standardizing on that.

@MikeRyanDev
Copy link
Member

Thank you! These build-related PRs are very welcome.

@MikeRyanDev MikeRyanDev merged commit b9776ea into ngrx:master Jul 16, 2017
@MikeRyanDev
Copy link
Member

Regarding the contributing doc, I'll gladly accept a PR updating it.

@bfricka bfricka deleted the dev/build-improvements-2 branch July 16, 2017 01:29
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.

3 participants