chore: upgrade to typescript 3.0.x and lerna 3.1.x #1626

raymondfeng · 2018-08-20T19:19:45Z

Upgrade to TypeScript 3.0.x and lerna 3.1.x

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 examples/* were updated

virkt25

👏 awesome PR! In a followup PR now we can explore using TS Project Linking to see if it improves build times.

Just have a few questions to help me better understand some of the changes.

virkt25 · 2018-08-21T03:54:35Z

bin/update-template-deps.js

+/**
+ * A dummy command to get filtered packages
+ */
+class NopCommand extends Command {


How does this help us get filtered packages?

The base command has the default steps to set up filter packages. We execute a no-op command to get such information.

virkt25 · 2018-08-21T03:55:19Z

bin/update-template-deps.js

-// Load dependencies from `packages/build/package.json`
-const buildDeps = require(path.join(rootPath, 'packages/build/package.json'))
-  .dependencies;
+  await cmd; // Execute the command


just an await executes the command? How?

The constructor returns an instance of the command, which has then() to function as a promise to run it.

virkt25 · 2018-08-21T03:56:56Z

packages/build/.prettierrc

@@ -1,6 +0,0 @@
-{


Why are we getting rid of this file?

It creates inconsistency against loopback-next root level prettier.

virkt25 · 2018-08-21T03:59:54Z

packages/build/package.json

@@ -39,13 +39,7 @@
    "lb-clean": "./bin/run-clean.js"
  },
  "scripts": {
-    "lint": "npm run prettier:check",


Why are we getting rid of linting in this package?

No, it's just redundant as we already have npm run lint at loopback-next level.

Oh. I thought all packages had those but just double checked and they don't. 👍

virkt25 · 2018-08-21T04:01:11Z

packages/cli/test/acceptance/app-run.acceptance.js

    scope: scope,
    loglevel: 'silent',
  });
-  return cmd.run();
+  await cmd;


How does just awaiting a command trigger it? 🤔

Please note that cmd is a promise-like object when runs the command itself.

virkt25 · 2018-08-21T04:06:10Z

commitlint.config.js

-    '@commitlint/config-conventional',
-    '@commitlint/config-lerna-scopes',
-  ],
+  extends: ['@commitlint/config-conventional', './bin/config-lerna-scopes'],


I know the switch to away from @commitlint/config-lerna-scopes is because it doesn't yet support Lerna 3.x BUT they have a PR in progress to add 3.x support ... I don't think we should introduce our own script for the package as it becomes something for us to maintain. We can split this PR to land the TypeScript upgrade and give commitlint some time to upgrade (knowing their history and the current progress of the PR I'd say a week tops) and we won't have to have a custom script to maintain?

commitlint PR: https://github.com/marionebl/commitlint/pull/406

Haha, I didn't see the PR and was trying to submit one. It's actually not too bad to maintain some of the commit rules for loopback-next itself though, IMO.

raymondfeng · 2018-08-21T14:43:31Z

package.json

  },
  "scripts": {
-    "postinstall": "lerna bootstrap",
+    "postinstall": "lerna bootstrap --no-ci",


For the record, lerna 3.x forces npm ci instead of npm i for CI envs. But npm ci fails as we don't check in package-lock.json files. It's not working to set the ci flag to false in lerna.json as the value is overridden.

Thanks for the explanation! I had to look at the docs to find the info. Would help other reviewers. Is there an issue regarding the flag in lerna.json not working? Would be good to remove it eventually but not a blocker for this PR.

See lerna/lerna#1449

Cross-posting lerna/lerna#1449 (comment)

Next release should fix this for you properly.

AFAICT, the next release has been already published, see https://github.com/lerna/lerna/releases/tag/v3.1.3

Bug Fixes

add: Avoid passing bad scope to pacote
global-options: Move env defaults to command superclass

@raymondfeng Do you have bandwidth to update our scripts and remove --no-ci in favor of settingci: false in lerna.json?

LOL, the follow-up pull request has been already landed, see #1632.

Please ignore my comment above.

raymondfeng · 2018-08-21T14:48:42Z

@virkt25 PTAL

virkt25 · 2018-08-21T14:58:01Z

In terms of the changes I don't have any more comments or concerns except I don't think we should be creating our own script for the packages given the PR in progress. It's one less thing for us to maintain.

It's actually not too bad to maintain some of the commit rules for loopback-next itself though, IMO.
We do maintain the rules, the script is a utility to get valid scopes so I don't think we should own this.

shimks

LGTM as far as I can tell. I wanted to look at some docs behind @lerna/project but lerna doesn't seem to have any...

raymondfeng · 2018-08-21T15:08:15Z

I don't think we should be creating our own script for the packages given the PR in progress. It's one less thing for us to maintain.

Considering the code is tiny and it also gives us flexibility to introduce extra scopes, such as examples, I won't be too worried if we have it in the code base.

We can revert it back to the config-lerna-scopes module once a new version is released instead of waiting if the team feel strongly about it.

virkt25 · 2018-08-21T15:19:59Z

Considering the code is tiny and it also gives us flexibility to introduce extra scopes, such as examples, I won't be too worried if we have it in the code base.

Adding additional scopes would go against conventional commits. It should either be a package name or nothing.

We can revert it back to the config-lerna-scopes module once a new version is released instead of waiting if the team feel strongly about it.

That sounds reasonable to me. Can you please add a comment to commitlint.config.js to remind us to the config-lerna-scopes package once it's released with the newest version and the script can be deleted. Or maybe an issue.

raymondfeng requested a review from bajtos as a code owner August 20, 2018 19:19

raymondfeng changed the title ~~chore: upgrade to typescript 3.0.x~~ chore: upgrade to typescript 3.0.x and lerna 3.1.x Aug 20, 2018

raymondfeng force-pushed the upgrade-to-typescript-3.0.x branch 4 times, most recently from c0bde0b to 466b9fd Compare August 20, 2018 23:53

virkt25 reviewed Aug 21, 2018

View reviewed changes

raymondfeng force-pushed the upgrade-to-typescript-3.0.x branch 2 times, most recently from e6c2823 to 9afbefb Compare August 21, 2018 05:48

raymondfeng commented Aug 21, 2018

View reviewed changes

chore: upgrade to typescript 3.0.x

694f298

raymondfeng force-pushed the upgrade-to-typescript-3.0.x branch from 9afbefb to 7e9de76 Compare August 21, 2018 14:48

shimks approved these changes Aug 21, 2018

View reviewed changes

chore: upgrade to lerna 3.x

b9129f5

raymondfeng force-pushed the upgrade-to-typescript-3.0.x branch from 7e9de76 to b9129f5 Compare August 21, 2018 15:49

virkt25 approved these changes Aug 21, 2018

View reviewed changes

raymondfeng merged commit 12c24a8 into master Aug 21, 2018

raymondfeng deleted the upgrade-to-typescript-3.0.x branch August 21, 2018 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: upgrade to typescript 3.0.x and lerna 3.1.x #1626

chore: upgrade to typescript 3.0.x and lerna 3.1.x #1626

raymondfeng commented Aug 20, 2018 •

edited

virkt25 left a comment

virkt25 Aug 21, 2018

raymondfeng Aug 21, 2018

virkt25 Aug 21, 2018

raymondfeng Aug 21, 2018

virkt25 Aug 21, 2018

raymondfeng Aug 21, 2018

virkt25 Aug 21, 2018

raymondfeng Aug 21, 2018

virkt25 Aug 21, 2018

virkt25 Aug 21, 2018

raymondfeng Aug 21, 2018

virkt25 Aug 21, 2018

virkt25 Aug 21, 2018

raymondfeng Aug 21, 2018

raymondfeng Aug 21, 2018

virkt25 Aug 21, 2018

raymondfeng Aug 21, 2018

bajtos Aug 23, 2018

bajtos Aug 23, 2018

raymondfeng commented Aug 21, 2018

virkt25 commented Aug 21, 2018

shimks left a comment

raymondfeng commented Aug 21, 2018

virkt25 commented Aug 21, 2018

chore: upgrade to typescript 3.0.x and lerna 3.1.x #1626

chore: upgrade to typescript 3.0.x and lerna 3.1.x #1626

Conversation

raymondfeng commented Aug 20, 2018 • edited

Checklist

virkt25 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

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

Choose a reason for hiding this comment

raymondfeng commented Aug 21, 2018

virkt25 commented Aug 21, 2018

shimks left a comment

Choose a reason for hiding this comment

raymondfeng commented Aug 21, 2018

virkt25 commented Aug 21, 2018

raymondfeng commented Aug 20, 2018 •

edited