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

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

Merged
merged 11 commits into from
Oct 5, 2021
24 changes: 24 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"ansi-colors": "4.1.1",
"autoprefixer": "10.2.5",
"concurrently": "^6.0.0",
Expand Down
1 change: 1 addition & 0 deletions scripts/license.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const entryDeps = [
'@rollup/plugin-commonjs',
'@rollup/plugin-json',
'@rollup/plugin-node-resolve',
'@yarnpkg/lockfile',
'ansi-colors',
'autoprefixer',
'css',
Expand Down
95 changes: 79 additions & 16 deletions src/cli/telemetry/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export const prepareData = async (
component_count: number = undefined
): Promise<d.TrackableData> => {
const { typescript, rollup } = coreCompiler.versions || { typescript: 'unknown', rollup: 'unknown' };
const { packages } = await getInstalledPackages(sys, config);
const { packages, packages_no_versions } = await getInstalledPackages(sys, config);
const targets = await getActiveTargets(config);
const yarn = isUsingYarn(sys);
const stencil = coreCompiler.version || 'unknown';
Expand All @@ -124,10 +124,12 @@ export const prepareData = async (
component_count,
targets,
packages,
packages_no_versions,
arguments: config.flags.args,
task: config.flags.task,
stencil,
system,
system_major: getMajorVersion(system),
os_name,
os_version,
cpu_model,
Expand All @@ -139,24 +141,28 @@ export const prepareData = async (
};

/**
* Reads package-lock.json and package.json files in order to cross references the dependencies and devDependencies properties. Pull the current installed version of each package under the @stencil, @ionic, and @capacitor scopes.
* Reads package-lock.json, yarn.lock, and package.json files in order to cross reference
* the dependencies and devDependencies properties. Pulls up the current installed version
* of each package under the @stencil, @ionic, and @capacitor scopes.
* @returns string[]
*/
async function getInstalledPackages(sys: d.CompilerSystem, config: d.Config): Promise<{ packages: string[] }> {
async function getInstalledPackages(
sys: d.CompilerSystem,
config: d.Config
): Promise<{ packages: string[]; packages_no_versions: string[] }> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

let packages: string[] = [];
let packageLockJson: any;
let packages_no_versions: string[] = [];
const yarn = isUsingYarn(sys);

try {
// Read package.json and package-lock.json
const appRootDir = sys.getCurrentDirectory();

const packageJson: d.PackageJsonData = await tryFn(readJson, sys.resolvePath(appRootDir + '/package.json'));

packageLockJson = await tryFn(readJson, sys.resolvePath(appRootDir + '/package-lock.json'));
const packageJson: d.PackageJsonData = await tryFn(readJson, sys, sys.resolvePath(appRootDir + '/package.json'));

// They don't have a package.json for some reason? Eject button.
if (!packageJson) {
return { packages };
return { packages, packages_no_versions };
}

const rawPackages: [string, string][] = Object.entries({
Expand All @@ -170,20 +176,67 @@ async function getInstalledPackages(sys: d.CompilerSystem, config: d.Config): Pr
([k]) => k.startsWith('@stencil/') || k.startsWith('@ionic/') || k.startsWith('@capacitor/')
);

packages = packageLockJson
? ionicPackages.map(
([k, v]) =>
`${k}@${packageLockJson?.dependencies[k]?.version ?? packageLockJson?.devDependencies[k]?.version ?? v}`
)
: ionicPackages.map(([k, v]) => `${k}@${v}`);
try {
packages = yarn ? await yarnPackages(sys, ionicPackages) : await npmPackages(sys, ionicPackages);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

} catch (e) {
packages = ionicPackages.map(([k, v]) => `${k}@${v.replace('^', '')}`);
}

packages_no_versions = ionicPackages.map(([k]) => `${k}`);

return { packages };
return { packages, packages_no_versions };
} catch (err) {
hasDebug(config) && console.error(err);
return { packages };
return { packages, packages_no_versions };
}
}

/**
* Visits the npm lock file to find the exact versions that are installed
* @param sys The system where the command is invoked
* @param ionicPackages a list of the found packages matching `@stencil`, `@capacitor`, or `@ionic` from the package.json file.
* @returns an array of strings of all the packages and their versions.
*/
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'));
Copy link
Contributor

Choose a reason for hiding this comment

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

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


return ionicPackages.map(([k, v]) => {
let version = packageLockJson?.dependencies[k]?.version ?? packageLockJson?.devDependencies[k]?.version ?? v;
version = version.includes('file:') ? sanitizeDeclaredVersion(v) : version;
return `${k}@${version}`;
});
}

/**
* Visits the yarn lock file to find the exact versions that are installed
* @param sys The system where the command is invoked
* @param ionicPackages a list of the found packages matching `@stencil`, `@capacitor`, or `@ionic` from the package.json file.
* @returns an array of strings of all the packages and their versions.
*/
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this to yarnLockYml or something similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

version = version.includes('undefined') ? sanitizeDeclaredVersion(identifiedVersion) : version;
return `${k}@${version}`;
});
}

/**
* This function is used for fallback purposes, where an npm or yarn lock file doesn't exist in the consumers directory.
* This will strip away ^ and ~ from the declared package versions in a package.json.
* @param version the raw semver pattern identifier version string
* @returns a cleaned up representation without any qualifiers
*/
function sanitizeDeclaredVersion(version: string) {
return version.replace(/[*^~]/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the intent

}

/**
* If telemetry is enabled, send a metric via IPC to a forked process for uploading.
*/
Expand Down Expand Up @@ -287,3 +340,13 @@ export async function enableTelemetry(sys: d.CompilerSystem): Promise<boolean> {
export async function disableTelemetry(sys: d.CompilerSystem): Promise<boolean> {
return await updateConfig(sys, { 'telemetry.stencil': false });
}

/**
* Takes in a semver string in order to return the major version.
* @param version The fully qualified semver version
* @returns a string of the major version
*/
function getMajorVersion(version: string): string {
const parts = version.split('.');
return parts[0];
}
6 changes: 6 additions & 0 deletions src/cli/telemetry/test/telemetry.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,11 @@ describe('prepareData', () => {
os_name: '',
os_version: '',
packages: [],
packages_no_versions: [],
rollup: 'unknown',
stencil: 'unknown',
system: 'in-memory __VERSION:STENCIL__',
system_major: 'in-memory __VERSION:STENCIL__',
targets: [],
task: undefined,
typescript: 'unknown',
Expand Down Expand Up @@ -178,9 +180,11 @@ describe('prepareData', () => {
os_name: '',
os_version: '',
packages: [],
packages_no_versions: [],
rollup: 'unknown',
stencil: 'unknown',
system: 'in-memory __VERSION:STENCIL__',
system_major: 'in-memory __VERSION:STENCIL__',
targets: ['www'],
task: undefined,
typescript: 'unknown',
Expand Down Expand Up @@ -208,9 +212,11 @@ describe('prepareData', () => {
os_name: '',
os_version: '',
packages: [],
packages_no_versions: [],
rollup: 'unknown',
stencil: 'unknown',
system: 'in-memory __VERSION:STENCIL__',
system_major: 'in-memory __VERSION:STENCIL__',
targets: ['www'],
task: undefined,
typescript: 'unknown',
Expand Down
4 changes: 4 additions & 0 deletions src/declarations/stencil-public-compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,10 @@ export interface CompilerSystem {
*/
normalizePath(p: string): string;
onProcessInterrupt?(cb: () => void): void;
parseYarnLockFile?: (content: string) => {
type: 'success' | 'merge' | 'conflict';
object: any;
};
platformPath: PlatformPath;
/**
* All return paths are full normalized paths, not just the basenames. Always returns an array, does not throw.
Expand Down
4 changes: 4 additions & 0 deletions src/sys/node/node-sys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { cpus, freemem, platform, release, tmpdir, totalmem } from 'os';
import { createHash } from 'crypto';
import exit from 'exit';
import fs from 'graceful-fs';
import { parse as parseYarnLockFile } from '@yarnpkg/lockfile';
import { isFunction, isPromise, normalizePath } from '@utils';
import { NodeLazyRequire } from './node-lazy-require';
import { NodeResolveModule } from './node-resolve-module';
Expand Down Expand Up @@ -256,6 +257,9 @@ export function createNodeSys(c: { process?: any } = {}) {
});
});
},
parseYarnLockFile(content: string) {
return parseYarnLockFile(content);
},
isTTY() {
return !!process?.stdout?.isTTY;
},
Expand Down