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

Get rid of warnings for dual esm and cjs packages of the same version #1890

Merged
merged 2 commits into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion packages/util/src/versionDetect.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

/// <reference types="@polkadot/dev-test/globals.d.ts" />

import { detectPackage } from './versionDetect.js';
import { detectPackage, POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG } from './versionDetect.js';

describe('detectPackage', (): void => {
const PKG = '@polkadot/util';
Expand Down Expand Up @@ -70,3 +70,41 @@ The following conflicting packages were found:
spy.mockRestore();
});
});

describe('detectPackageEsmCjsNoWarnings', (): void => {
const PKG = '@polkadot/wasm-crypto';
const VER1 = '9.8.0-beta.45';
const PATH = '/Users/jaco/Projects/polkadot-js/api/node_modules/@polkadot/api/node_modules/@polkadot/wasm-crypto';

it('should not log when there are concurrent esm and cjs versions of the same package with the same version number and warnings are disabled', (): void => {
const spy = jest.spyOn(console, 'warn');
const pkgEsm = { name: PKG, path: PATH, type: 'esm', version: VER1 };
const pkgCjs = { name: PKG, path: `${PATH}/cjs`, type: 'cjs', version: VER1 };

process.env[POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG] = '1';
detectPackage(pkgEsm, false, []);
detectPackage(pkgCjs, false, []);

expect(spy).not.toHaveBeenCalled();
spy.mockRestore();
});
});

describe('detectPackageEsmCjs', (): void => {
const PKG = '@polkadot/wasm-crypto-wasm';
const VER1 = '9.8.0-beta.45';
const PATH = '/Users/jaco/Projects/polkadot-js/api/node_modules/@polkadot/api/node_modules/@polkadot/wasm-crypto-wasm';

it('should log when there are concurrent esm and cjs versions of the same package with the same version number and warnings are not disabled', (): void => {
const spy = jest.spyOn(console, 'warn');
const pkgEsm = { name: PKG, path: PATH, type: 'esm', version: VER1 };
const pkgCjs = { name: PKG, path: `${PATH}/cjs`, type: 'cjs', version: VER1 };

process.env[POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG] = undefined;
detectPackage(pkgEsm, false, []);
detectPackage(pkgCjs, false, []);

expect(spy).toHaveBeenCalled();
spy.mockRestore();
});
});
13 changes: 11 additions & 2 deletions packages/util/src/versionDetect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ type FnString = () => string | undefined;

const DEDUPE = 'Either remove and explicitly install matching versions or dedupe using your package manager.\nThe following conflicting packages were found:';

export const POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG = 'POLKADOTJS_DISABLE_ESM_CJS_WARNING';

/** @internal */
function getEntry (name: string): VersionPath[] {
const _global = xglobal as PjsGlobal;
Expand Down Expand Up @@ -105,7 +107,7 @@ function warn <T extends { version: string }> (pre: string, all: T[], fmt: (vers
/**
* @name detectPackage
* @summary Checks that a specific package is only imported once
* @description A `@polkadot/*` version detection utility, checking for one occurence of a package in addition to checking for ddependency versions.
* @description A `@polkadot/*` version detection utility, checking for one occurrence of a package in addition to checking for dependency versions.
*/
export function detectPackage ({ name, path, type, version }: PackageInfo, pathOrFn?: FnString | string | false | null, deps: PackageInfo[] = []): void {
if (!name.startsWith('@polkadot')) {
Expand All @@ -116,7 +118,14 @@ export function detectPackage ({ name, path, type, version }: PackageInfo, pathO

entry.push({ path: getPath(path, pathOrFn), type, version });

if (entry.length !== 1) {
// if we have more than one entry at DIFFERENT version types then warn. If there is more than one entry at the same
// version and ESM/CJS dual warnings are disabled, then do not display warnings
const entriesSameVersion = entry.every((e) => e.version === version);
const esmCjsWarningDisabled = POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG in process.env && process.env[POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG] === '1';
Copy link
Member

@jacogr jacogr Nov 17, 2023

Choose a reason for hiding this comment

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

Suggested change
const esmCjsWarningDisabled = POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG in process.env && process.env[POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG] === '1';
const esmCjsWarningDisabled = xglobal.process?.env?.[POLKADOTJS_DISABLE_ESM_CJS_WARNING_FLAG] === '1';

This ensures we work in Node, browser and Deno environments. (The Deno has a CI failure)

Additionally it just simplifies the check slightly.

Copy link
Member

@jacogr jacogr Nov 17, 2023

Choose a reason for hiding this comment

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

(PS: I would just merge in master as well, it should make it slightly easier, with the above suggestion, after #1894 - always a bit of a nightmare with the different environments and process...)

const multipleEntries = entry.length !== 1;
const disableWarnings = esmCjsWarningDisabled && entriesSameVersion;

if (multipleEntries && !disableWarnings) {
warn(`${name} has multiple versions, ensure that there is only one installed.`, entry, formatVersion);
} else {
const mismatches = deps.filter((d) => d && d.version !== version);
Expand Down
Loading