Skip to content

feat: run tsc relative to the root directory of loopback-next monorepo #1189

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.

Sign up for GitHub

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

Jump to bottom
Merged
raymondfeng merged 1 commit into from
Mar 29, 2018
Merged

feat: run tsc relative to the root directory of loopback-next monorepo #1189

raymondfeng merged 1 commit into from
Mar 29, 2018

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Mar 26, 2018 •
edited
Loading

This PR adds run-lerna script to allow LERNA_ROOT_PATH to be passed into lerna commands.

The run-lerna script adds the following logic:

  • Set LERNA_ROOT_PATH env var

Fixes #1010

Checklist

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

@raymondfeng raymondfeng requested a review from bajtos as a code owner March 26, 2018 20:14
@raymondfeng raymondfeng mentioned this pull request Mar 26, 2018
Closed
2 tasks
@raymondfeng raymondfeng force-pushed the add-lb-env branch 4 times, most recently from 2e6b0a3 to 7f5bce4 Compare March 27, 2018 04:10
@bajtos
Copy link
Member

bajtos commented Mar 27, 2018

+1 for adding bin/run-lerna.js, it's a decent workaround for lerna/lerna#1343.

-1 for implementing bin/run-env.js, it feels like reinventing a wheel to me. Have you tried cross-env? What are the arguments against cross-env in favor of maintaining our own implementation?

bajtos
bajtos suggested changes Mar 27, 2018
View reviewed changes
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.

Besides my comment above, please update loopback/build's README to mention the new script(s).

@raymondfeng raymondfeng force-pushed the add-lb-env branch 4 times, most recently from 17e91dd to f1fe2e5 Compare March 27, 2018 20:04
@raymondfeng
Copy link
Contributor Author

raymondfeng commented Mar 27, 2018

@bajtos I have implemented the support for #1010. PTAL.

@raymondfeng raymondfeng changed the title feat(build): add lb-env script to set up env vars feat: run tsc relative the root directory of loopback-next monorepo Mar 27, 2018
@raymondfeng raymondfeng changed the title feat: run tsc relative the root directory of loopback-next monorepo feat: run tsc relative to the root directory of loopback-next monorepo Mar 28, 2018
@raymondfeng raymondfeng mentioned this pull request Mar 28, 2018
Merged
7 tasks
bajtos
bajtos suggested changes Mar 29, 2018
View reviewed changes
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.

If you are ok with removing isVSCode per my comment above, then the part of this patch adding LERNA_ROOT_PATH only can be landed without further review.

bin/run-lerna.js Outdated
process.env.LERNA_ROOT_PATH = rootPath;
let args = argv.slice(2);

// Force lerna command runs in non-parallel mode for VSCode builds
Copy link
Member

@bajtos bajtos Mar 29, 2018

Choose a reason for hiding this comment

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

I am not very happy with this solution, it will significantly slow down builds triggered from VS Code. Can we either leave this part out of this initial pull request?

Here is what I would like us to eventually implement:

  • At minimum, have two VSCode tasks - one for the usual "fast" build that reports broken error paths, anther for a "slow" build that works well with PROBLEMS window.
  • Ideally, we should invoke typescript in parallel via some sort of a tool that can merge the output in a clever way, or perhaps tell Lerna to not print those prefixes.

Either way, I'd like to have the option to run the build in parallel mode, it stays my default mode most of the time.

Copy link
Contributor Author

@raymondfeng raymondfeng Mar 29, 2018

Choose a reason for hiding this comment

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

I misunderstood what you want for VSCode.

Here is what I found out:

I'll remove the VSCode check for now.

Copy link
Contributor Author

@raymondfeng raymondfeng Mar 29, 2018

Choose a reason for hiding this comment

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

I contributed a patch to lerna - lerna/lerna#1352

@raymondfeng
chore: add run-lerna script to run lerna with repo root
7a620b2 
The script sets `LERNA_ROOT_PATH` to the root directory of loopback-next
monorepo and run `lerna` commands. It also detects VSCode and disables
parallel mode.
@raymondfeng raymondfeng merged commit 323f253 into master Mar 29, 2018
@raymondfeng raymondfeng mentioned this pull request Mar 30, 2018
Merged
7 tasks
@bajtos
Copy link
Member

bajtos commented Apr 2, 2018

👏

@bajtos bajtos deleted the add-lb-env branch June 5, 2018 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bajtos bajtos bajtos requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@raymondfeng raymondfeng

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@raymondfeng @bajtos