feat(telemetry) adding yarn 1 support, sanitizing data pre-flight #3082

splitinfinities · 2021-09-27T16:37:55Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

We're unable to understand how Yarn users work with Stencil today.
GitHub Issue Number: N/A

What is the new behavior?

This PR allows the yarn.lock file to be read.
Sanitizes two data points pre-flight - the system version (Node/Deno) and the packages array.

Does this introduce a breaking change?

Yes
No

Testing

Other information

rwaskiewicz

I took a first pass on this, but didn't get around to testing it going much deeper than the surface level. LMK what you think

src/cli/telemetry/telemetry.ts

rwaskiewicz · 2021-09-27T19:27:59Z

src/cli/telemetry/telemetry.ts

+        packages = await npmPackages(sys, ionicPackages);
+      } else if (isUsingYarn(sys)) {
+        packages = await yarnPackages(sys, ionicPackages);
+      } else {


Doesn't isUsingYarn always return a boolean? I'm not sure I see the scenario where this else block is entered (since we test both isUsingYarn in the else if condition and it's negation in the if condition clause)

In fact, I think this could be refactored as such

// NOTE to not be committed: I pulled this out to keep our `try` block tiny const isUsingYarn = isUsingYarn(sys); try { packages = isUsingYarn ? await yarnPackages(sys, ionicPackages) : await npmPackages(sys, ionicPackages); } catch (e) { packages = ionicPackages.map(([k, v]) => `${k}@${v.replace('^', '')}`); }

rwaskiewicz · 2021-09-27T19:35:42Z

package.json

@@ -79,6 +79,8 @@
    "@types/sizzle": "^2.3.2",
    "@types/webpack": "^4.41.26",
    "@types/ws": "^7.4.0",
+    "@types/yarnpkg__lockfile": "^1.1.5",
+    "@yarnpkg/lockfile": "^1.1.0",


Can you add @yarnpkg/lockfile to

stencil/scripts/license.ts

Line 12 in 66d0476

'@rollup/plugin-node-resolve',

As a dev dependency, how is this getting rolled up and put in the final compiler output?

It's a part of the compiler process, since anything within the node sys gets bundled up and shipped. Similar to how we bundle TypeScript or Rollup.

rwaskiewicz · 2021-09-27T19:37:17Z

src/cli/telemetry/telemetry.ts

+ */
+async function yarnPackages(sys: d.CompilerSystem, ionicPackages: [string, string][]) {
+  const appRootDir = sys.getCurrentDirectory();
+  const yarnLock = sys.readFileSync(appRootDir + '/yarn.lock');


We should use path.join (or the sys equivalent) here, to make the path delimiter OS-agonstic

rwaskiewicz · 2021-09-27T19:37:21Z

src/cli/telemetry/telemetry.ts

+ */
+async function npmPackages(sys: d.CompilerSystem, ionicPackages: [string, string][]) {
+  const appRootDir = sys.getCurrentDirectory();
+  const packageLockJson: any = await tryFn(readJson, sys, sys.resolvePath(appRootDir + '/package-lock.json'));


We should use path.join (or the sys equivalent) here, to make the path delimiter OS-agonstic

src/cli/telemetry/telemetry.ts

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

rwaskiewicz

Tested on Windows + MacOS, looking good 😎

Left a few comments/nitpicks around typings, but nothing major

rwaskiewicz · 2021-09-30T19:30:45Z

package.json

@@ -79,6 +79,8 @@
    "@types/sizzle": "^2.3.2",
    "@types/webpack": "^4.41.26",
    "@types/ws": "^7.4.0",
+    "@types/yarnpkg__lockfile": "^1.1.5",
+    "@yarnpkg/lockfile": "^1.1.0",


As a dev dependency, how is this getting rolled up and put in the final compiler output?

rwaskiewicz · 2021-09-30T19:35:18Z

src/cli/telemetry/telemetry.ts

-        )
-      : ionicPackages.map(([k, v]) => `${k}@${v}`);
+    try {
+      packages = yarn ? await yarnPackages(sys, ionicPackages) : await npmPackages(sys, ionicPackages);


For my own knowledge, in what cases do we expect either of these await calls to throw?

If the lock files cannot be read (doesn't exist/incorrect file structure), that's when these should throw.

rwaskiewicz · 2021-09-30T19:36:10Z

src/cli/telemetry/telemetry.ts

+async function getInstalledPackages(
+  sys: d.CompilerSystem,
+  config: d.Config
+): Promise<{ packages: string[]; packages_no_versions: string[] }> {


Can we camelCase pacakges_no_versions please? I think we'd want to be using camelCase until it's time to send the snake_case value off to the aggregation service

rwaskiewicz · 2021-09-30T20:03:57Z

src/cli/telemetry/telemetry.ts

+ * @returns a cleaned up representation without any qualifiers
+ */
+function sanitizeDeclaredVersion(version: string) {
+  return version.replace(/[*^~]/g, '');


This is doing a global replace, which is probably fine?

Yes, that was the intent

rwaskiewicz · 2021-09-30T20:06:56Z

src/cli/telemetry/telemetry.ts

+async function yarnPackages(sys: d.CompilerSystem, ionicPackages: [string, string][]) {
+  const appRootDir = sys.getCurrentDirectory();
+  const yarnLock = sys.readFileSync(sys.resolvePath(appRootDir + '/yarn.lock'));
+  const packageLockJson = sys.parseYarnLockFile(yarnLock);


Should we rename this to yarnLockYml or something similar?

Can you walk me through the success/failure cases of this being read? Will packageLockJson (as its named today) always be a truthy value, even when the reading fails?

If reading fails, it throws and resolves back to the parent try/catch block. All other paths are truthy as you say.

rwaskiewicz · 2021-10-01T11:55:47Z

src/cli/telemetry/telemetry.ts

+
+  return ionicPackages.map(([k, v]) => {
+    const identifiedVersion = `${k}@${v}`;
+    let version = packageLockJson.object[identifiedVersion]?.version;


Since object is of type any, can we handle the case where it is undefined/null?

We would handle that above, within the getInstalledPackages try/catch block.

src/cli/telemetry/telemetry.ts

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

src/cli/telemetry/telemetry.ts

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

splitinfinities · 2021-10-05T16:54:05Z

Note that yarn 2+ are not supported

this pr moves yarn dependencies from the `dependencies` section of our package-lock.json to `dev-dependencies`. In #3082, we didn't use the npm cli to move these from dependencies->dev-dependencies during iteration of the feature. this PR puts them in the correct place.

splitinfinities added 2 commits September 27, 2021 11:32

telemetry(yarn) adding yarn support, sanitizing data pre-flight

932301a

Relocate the yarn package deps

3c379a8

splitinfinities requested a review from a team September 27, 2021 16:37

nix the export on getMajorVersion

5bcef46

splitinfinities changed the title ~~telemetry(yarn) adding yarn support, sanitizing data pre-flight~~ feat(telemetry) adding yarn support, sanitizing data pre-flight Sep 27, 2021

splitinfinities self-assigned this Sep 27, 2021

rwaskiewicz suggested changes Sep 27, 2021

View reviewed changes

splitinfinities and others added 2 commits September 27, 2021 15:41

Update src/cli/telemetry/telemetry.ts

0178d84

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Updates from Ryan

a2eacd5

rwaskiewicz suggested changes Oct 1, 2021

View reviewed changes

splitinfinities and others added 4 commits October 4, 2021 16:28

Update src/cli/telemetry/telemetry.ts

64dcdc8

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Update src/cli/telemetry/telemetry.ts

17878ba

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Update src/cli/telemetry/telemetry.ts

dc3fa30

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Feedback from Ryan

1b2084d

rwaskiewicz reviewed Oct 5, 2021

View reviewed changes

src/cli/telemetry/telemetry.ts Outdated Show resolved Hide resolved

Update src/cli/telemetry/telemetry.ts

3869acf

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

rwaskiewicz approved these changes Oct 5, 2021

View reviewed changes

Merge branch 'master' into add-yarn-and-expand-data

1a5b657

splitinfinities changed the title ~~feat(telemetry) adding yarn support, sanitizing data pre-flight~~ feat(telemetry) adding yarn 1 support, sanitizing data pre-flight Oct 5, 2021

rwaskiewicz merged commit 07f69cb into master Oct 5, 2021

rwaskiewicz deleted the add-yarn-and-expand-data branch October 5, 2021 17:01

rwaskiewicz mentioned this pull request Oct 8, 2021

chore(package-lock): move yarn dependencies #3097

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(telemetry) adding yarn 1 support, sanitizing data pre-flight #3082

feat(telemetry) adding yarn 1 support, sanitizing data pre-flight #3082

splitinfinities commented Sep 27, 2021

rwaskiewicz left a comment

rwaskiewicz Sep 27, 2021

rwaskiewicz Sep 27, 2021

rwaskiewicz Sep 30, 2021

splitinfinities Oct 4, 2021

rwaskiewicz Sep 27, 2021

rwaskiewicz Sep 27, 2021

rwaskiewicz left a comment

rwaskiewicz Sep 30, 2021

rwaskiewicz Sep 30, 2021

splitinfinities Oct 4, 2021

rwaskiewicz Sep 30, 2021

rwaskiewicz Sep 30, 2021

splitinfinities Oct 5, 2021

rwaskiewicz Sep 30, 2021

rwaskiewicz Sep 30, 2021

splitinfinities Oct 5, 2021

rwaskiewicz Oct 1, 2021

splitinfinities Oct 5, 2021

splitinfinities commented Oct 5, 2021

feat(telemetry) adding yarn 1 support, sanitizing data pre-flight #3082

feat(telemetry) adding yarn 1 support, sanitizing data pre-flight #3082

Conversation

splitinfinities commented Sep 27, 2021

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

rwaskiewicz 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

rwaskiewicz 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

splitinfinities commented Oct 5, 2021