npm package contains minified files #620

joepie91 · 2022-03-15T18:31:00Z

Is your feature request related to a problem? Please describe.

The package as it is published on npm currently contains multiple minified files; this is problematic from a security auditing perspective, as minified code is difficult to audit. This is particularly problematic because of the often security-sensitive nature of the uuid module.

A more detailed explanation of the problem with minified builds and why they are unnecessary to publish in the first place, can be found here.

Describe the solution you'd like

Removing all minified builds from the npm package in a future release, instead either a) providing instructions for users on how to obtain a minified build, or b) if necessary for certain CDNs, publishing the minified builds in a separate package that users wouldn't install into their project.

Describe alternatives you've considered

Reproducing from source: Requires auditing the entire build stack due to potential supply chain attacks, with build tools being able to modify the code going through them.
Prettifying minified code and auditing that: Cumbersome, unreliable because a lack of meaningful names makes it hard to understand the code correctly.
Just not using the minified files: For a reliable dependency audit, all files present in the dependency tree must be audited, because there is (AFAIK) no runtime-level mechanism to block certain files from being loaded in eg. a multi-stage attack.

Additional context

N/A

The text was updated successfully, but these errors were encountered:

broofa · 2022-03-15T23:24:19Z

Is your issue with the existence of multiple builds, or that [some of] those builds are minified?

The simplest solution (for us) would be to just turn off the compact flag in rollup.config.js. Keeps the existing package structure (probably won't break anything) while disabling minification.

joepie91 · 2022-03-15T23:59:51Z

While multiple builds do add auditing overhead and so are ideally avoided, it's not a huge issue, because most of the time you can simply diff between the builds and verify that the only differences are syntactic or in common boilerplate. The real problem lies with the minification - which is hard to read, and usually neither efficiently diffable against other builds nor against previous versions.

So I'd say that simply disabling minification would address my concern sufficiently well, yeah :)

ctavan · 2022-03-16T08:19:22Z

So essentially you want to get rid of the minified files in this folder: https://unpkg.com/browse/[email protected]/dist/umd/ ?

I think those UMD builds are really mostly a legacy from a time (~10 years ago) when bundlers were not the default for browser code.

I would be surprised if they are being used extensively these days outside of e.g. educational tools like http://repl.it and even those tools have good support for easily installing npm dependencies these days.

So I would actually vote for entirely removing the umd build in the next major version. We could still consider publishing them as a separate package, e.g. uuid-umd.

So adding this to the next major version milestone. Happy to accept pull requests as well :)

ctavan · 2022-03-16T08:21:59Z

Regarding multiple builds: I believe as long as we have both CommonJS and ESM in heavy use across browsers and server environments we won't be able to get rid of them.

LinusU · 2022-03-16T10:08:13Z

So I would actually vote for entirely removing the umd build in the next major version.

My vote goes towards this as well 👍

Regarding multiple builds: I believe as long as we have both CommonJS and ESM in heavy use across browsers and server environments we won't be able to get rid of them.

Personally, I would prefer to just rip the band-aid off and only ship ESM. It's supported by all supported versions of Node.js, and I believe that all major build tools for the browsers supports it, although I'm not 100% on that.

If we still need to support CommonJS I think that we should use a wrapping approach so that we don't need to include two builds for Node.js. This also has the upside that if you have multiple dependencies that are using uuid, and some have upgraded to ESM but some of them is still on CommonJS, then they will still only load one copy of the uuid module, instead of loading two separate copies.

Today we are actually shipping three versions for Node.js:

dist/esm-node/index.js which contains ESM syntax source code
dist/index.js which contains CommonJS syntax source code
wrapper.mjs which imports the CommonJS code, and exports it as an ESM module

I think we could remove dist/esm-node and only use the wrapper?

Circling back to ESM only though, I think that my preference would be to only ship two files to Npm: browser.js and node.js, both produced by Rollup (but not minified, just rolled up into one file, to simplify the Node vs. Browsers story) 🤔

All in all I think this is a great discussion to have!

joepie91 · 2022-03-16T14:21:11Z

I would be fine with either disabling minification or removing the minified build entirely, I don't have a strong preference. Either way the minified code would no longer be in the package :)

That having been said, however...

Personally, I would prefer to just rip the band-aid off and only ship ESM. It's supported by all supported versions of Node.js, and I believe that all major build tools for the browsers supports it, although I'm not 100% on that.

This is a really bad idea. Even leaving aside the problems with ESM as a module system (which are one of the reasons why "just migrate your code to ESM" is not a viable answer), ESM cannot be imported from CJS while it is possible the other way around.

So the real-world consequence of making the package ESM-only would be to essentially lock out most of the ecosystem (including breaking dependent trees), cause major headaches for nearly everybody using the module, and likely cause version stragglers like is happening with Sindre's packages. All the while there's really not any benefits to doing this in the first place.

If we still need to support CommonJS I think that we should use a wrapping approach so that we don't need to include two builds for Node.js.

If a wrapping approach is viable (I haven't checked the latest situation on this), then that would work, as it wouldn't break imports from CJS. The important thing here would be to not needlessly break compatibility, IMO.

LinusU · 2022-03-16T15:30:50Z

This is a really bad idea. Even leaving aside the problems with ESM as a module system (which are one of the reasons why "just migrate your code to ESM" is not a viable answer), ESM cannot be imported from CJS while it is possible the other way around.

I didn't really find any critique of ESM, just that CommonJS also can do everything that ESM can do. That might very well be true, and I actually used to agree that we shouldn't move away from CommonJS. But at this point that ship has sailed and I don't see anyone rolling back the change to ESM...

Anyhow, I don't see a rush in doing this for uuid, and I don't think that we should do it just because. Only if it makes it easier to maintain the package...

If a wrapping approach is viable (I haven't checked the latest situation on this), then that would work, as it wouldn't break imports from CJS. The important thing here would be to not needlessly break compatibility, IMO.

👍

broofa · 2022-03-16T18:48:13Z

This is a really bad idea.

I actually disagree with most of what you wrote there but the time for debating this has passed. ESM is the way forward. The uuid source has been ESM-based for... what... two years now? All we're talking about here is whether or not to remove the UMD build that is automatically generated by rollup.

One thing I do feel pretty strongly about is that module owners shouldn't have to be in the business of packaging code for different environments. The fact we provide different builds for node/browser/ESM/CJS is a testament to @ctavan's passion for making sure this stuff works on every conceivable platform.

To that end, I'd be fine taking a stance that we do ESM only, and if anyone wants something else, they need to figure out a toolchain that works for them.

ctavan · 2022-03-16T23:12:00Z

Wow, that thread escalated quickly 😂

While I would love to ease our pain as maintainers of this package, I think the uuid package has a long tradition of providing a good deal of backwards compatibility and I think this has actually helped keep the amount of noise on this repo relatively low since it mostly just works for most people…

I'd be afraid that restricting ourselves to only ESM would actually cause a lot of pain in the community, even if that may be the most modern solution at the moment.

So when we look at the support matrix right now we have:

	CommonJS	ESM
NodeJs	+	+
Browser	-	+

In practice the discussions have recently gone even in the opposite direction of considering to add a browser CommonJS build, see #616 (comment) (<- that comment contains 4 more references where folks have been requesting this).

I'm still convinced that adding a CommonJS browser build would be fundamentally wrong at this point in time. But I equally think that it's not realistic to get rid of the CommonJS Node build.

BTW to elaborate a bit on the export section in:

uuid/package.json

Lines 21 to 29 in 3a033f6

    
           "exports": { 
        
             ".": { 
        
               "node": { 
        
                 "module": "./dist/esm-node/index.js", 
        
                 "require": "./dist/index.js", 
        
                 "import": "./wrapper.mjs" 
        
               }, 
        
               "default": "./dist/esm-browser/index.js" 
        
             },

node.module is a pure Node.js ESM build which only gets chosen when people create bundles for node.js (e.g. with webpack or rollup). Yes, apparently people do that for serverless environments 🤷‍♂️ .
node.require gets loaded when you require('uuid') in Node.js
node.import gets loaded when you import 'uuid' in Node.js and this is actually a wrapper around the CommonJS module. This is a mitigation to the dual package hazard to ensure that only one instance of UUID gets instantiated even if in your dependency tree some modules are importing and some are require()ing uuid.
default is a browser ESM build that gets picked up by bundlers when building for the browser (we use ESM here to support proper tree shaking).

Sometimes people manage to misconfigure their bundlers so that the bundler is looking for a CommonJS build which we don't provide. I think this is always the wrong thing to do when using a bundler.

ljharb · 2022-03-17T19:48:02Z

To be clear, the only way a wrapping approach is viable for any package is if everything is in CJS, with maybe a thin ESM wrapper around it.

If the desire is to avoid a build process while not excluding the majority of the ecosystem (for evidence of this, look at any popular package that's gone ESM-only, and see how tiny a percentage of its users uses those versions), authoring in CJS directly is the only way to do that.

@ctavan the only tweak to your exports field is that if you want to also have your package work in node v13.0-13.6, you'd make the dot object be an array of "object" and "default filepath string".

ctavan · 2022-03-17T22:18:58Z

Thanks for chiming in @ljharb!

To be clear, the only way a wrapping approach is viable for any package is if everything is in CJS, with maybe a thin ESM wrapper around it.

Yes, that's what we're doing here for Node.js!

If the desire is to avoid a build process while not excluding the majority of the ecosystem (for evidence of this, look at any popular package that's gone ESM-only, and see how tiny a percentage of its users uses those versions), authoring in CJS directly is the only way to do that.

Agree! But since we want to provide an independent build that also makes sense for browser bundlers, ESM does have advantages for those environments (even if webpack managed to treeshake the CommonJS version of this module as well). I'm fine with keeping a build step, now that we have it.

@ctavan the only tweak to your exports field is that if you want to also have your package work in node v13.0-13.6, you'd make the dot object be an array of "object" and "default filepath string".

I was considering that back when I introduced pkg.exports but decided against supporting these intermediate non-LTS Node.js versions. When they were still current, a few folks were complaining, but "upgrade to the next LTS release" was a viable solution for all of them, it was usually just an oversight that they were still on a 13.x version.

ljharb · 2022-03-17T23:09:51Z

considering that the "default" key should work in node 13.3-13.6, and it's only v13.0-13.2 that will be completely unable to use the package, you're probably fine (altho in my packages i test on these versions too, just in case)

* chore(deps-dev): bump jest from 27.5.1 to 28.0.0 Bumps [jest](https://github.com/facebook/jest/tree/HEAD/packages/jest) from 27.5.1 to 28.0.0. - [Release notes](https://github.com/facebook/jest/releases) - [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md) - [Commits](https://github.com/facebook/jest/commits/v28.0.0/packages/jest) --- updated-dependencies: - dependency-name: jest dependency-type: direct:development update-type: version-update:semver-major ... --- This required some manual updates. The easy ones were: * Jest 28 no longer includes `jest-environment-jsdom` as an implicit dependency. Our unit and e2e tests both use this, so I've added an explicit dependency for it. * Jest 28 adds a new `github-actions` reporter available by default. This seems like an obvious win, so I went ahead and enabled it. * Jest 28 adds native support for a `--shard` argument; I've replaced the janky manual version our GitHub CI build was using with the new native support. I fixed the off-by-one issue with our sharded job naming while I was here. I'll update the `main` branch policy accordingly once this PR merges. The other three were more interesting; I've given them separate comments for ease of linking them. --- #### Jest 28 tries to use an ESM version of `uuid` by default Jest 28 attempts to respect `package.json` `exports` fields based on the test environment you're using. This means that for packages that export separate entry points for node vs browser environments, Jest will attempt to use the browser entry point when you're using `jest-environment-jsdom` and the node entry point when you're using `jest-environment-node`. This is a good change that we want to use in most cases; it keeps our test environment closer to a real browser environment. Unfortunately, one of our dependencies (`uuid`) only provides an ESM implementation (not CommonJS) for browsers, though it provides both ESM and CommonJS versions for node. [This comment on uuid#620 summarizes their exports matrix](uuidjs/uuid#620 (comment)), and [uuid#616's discussion](uuidjs/uuid#616) has some context on the issue. `uuid`'s `package.json` indicates that its ESM+browser entry point is what *all* browser cases should use, so Jest ends up trying to use that even though Jest tries to use a CommonJS+browser entry point where available. This produces hundreds of errors that look something like this: <details> <summary>Full example of an error</summary> ``` ● Test suite failed to run Jest encountered an unexpected token Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax. Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration. By default "node_modules" folder is ignored by transformers. Here's what you can do: • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it. • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config. • If you need a custom transformation specify a "transform" option in your config. • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option. You'll find more details and examples of these config options in the docs: https://jestjs.io/docs/configuration For information about custom transformations, see: https://jestjs.io/docs/code-transformation Details: C:\repos\accessibility-insights-web\node_modules\uuid\dist\esm-browser\index.js:1 ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export { default as v1 } from './v1.js'; ^^^^^^ SyntaxError: Unexpected token 'export' 1 | // Copyright (c) Microsoft Corporation. All rights reserved. 2 | // Licensed under the MIT License. > 3 | import { v4 } from 'uuid'; | ^ 4 | 5 | export type UUIDGenerator = () => string; 6 | at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1773:14) at Object.<anonymous> (src/common/uid-generator.ts:3:26) ``` </details> I considered a few options for addressing this: * We could wait and hope that `uuid` starts including a CommonJS + browser entry point, like uuidjs/uuid#616 proposes. Jest would start picking this up without any changes, but this would fail because such a `uuid` entry point would assume the presence of a browser `crypto.getRandomValues` API. We would then have to either wait for JSDOM to start providing this (probably via jsdom/jsdom#3352) or provide a polyfill as part of our test environment/test setup. This is probably not a good solution because it seems unlikely that `uuid` will add a CommonJS browser build; the most recent word I saw from `uuid`'s maintainers was that they were ["convinced that adding a CommonJS browser build would be fundamentally wrong at this point in time"](uuidjs/uuid#620 (comment)) * We could try to make ESM+Jest work (this would fix the immediate issue but would still require a `crypto` implementation). [Jest's native support for this is currently blocked on node/v8 issues](jestjs/jest#9430), but it's possible we could work around that with something like https://github.com/nicolo-ribaudo/jest-light-runner. * We could override the `exports` conditions that `jest-environment-jsdom` passes, per [the suggestion in the Jest v27 to v28 upgrade guide](https://jestjs.io/docs/upgrading-to-jest28#packagejson-exports). As I understand it, this would amount to creating our own custom Jest Test Environment wrapping `jest-environment-jsdom` and overriding its [`exportConditions`](https://github.com/facebook/jest/blob/v28.0.0/packages/jest-environment-jsdom/src/index.ts#L160) property to look for use `node` instead of `browser`. This is something we could do ourselves immediately and much less work than moving to ESM, but I didn't love this solution because I wanted to keep the proper export conditions in the cases where they aren't broken * We could use a Jest `moduleNameMapper` entry like `'^uuid$': require.resolve('uuid')` to force the use of a Node+CommonJS version of `uuid`. This works and is more scoped than overriding `exports` conditions, but has the downside that it is essentially a silent `yarn resolution`; it forces *every* dependency chain through `uuid` to use whatever version of `uuid` happens to be hoisted, even if some chains want older versions. This would make future `uuid` upgrades very dangerous. See uuidjs/uuid#616 (comment) for context. * The option I chose: create a [custom Jest resolver](9ad4e61) which overrides only the specific behavior of how Jest resolves the `uuid` dependency, forcing it use the CommonJS+node version despite `jest-environment-jsdom`'s export conditions. --- #### `jsdom` v17 assumes the availability of a global `TextEncoder`, which `jest-environment-jsdom` does not provide Because we updated how we pull in `jest-environment-jsdom`, we ended up updating the version of `jsdom` that we use in practice, so we started hitting this not-entirely-new issue. This one is described by jsdom/jsdom#2524 (comment) (the original issue is for a separate-but-related feature request, which was unfortunately hijacked). The symptom is an error of form `ReferenceError: TextEncoder is not defined` on a line importing `jsdom`. <details> <summary>Full example of an error</summary> ``` FAIL unit tests src/tests/unit/tests/injected/visualization/drawer.test.ts ● Test suite failed to run ReferenceError: TextEncoder is not defined 1 | // Copyright (c) Microsoft Corporation. All rights reserved. 2 | // Licensed under the MIT License. > 3 | import { JSDOM } from 'jsdom'; | ^ 4 | 5 | export class TestDocumentCreator { 6 | public static createTestDocument(html: string = ''): Document { at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/lib/encoding.js:2:21) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/lib/url-state-machine.js:5:34) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/lib/URL-impl.js:2:13) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/lib/URL.js:442:14) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/webidl2js-wrapper.js:3:13) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/index.js:3:34) at Object.<anonymous> (node_modules/jsdom/lib/api.js:7:19) at Object.<anonymous> (src/tests/unit/common/test-document-creator.ts:3:30) at Object.<anonymous> (src/tests/unit/tests/injected/visualization/drawer.test.ts:15:78) ``` </details> The issue is that we're trying to import `jsdom` from within a `jest-environment-jsdom` environment. `jsdom` uses a global `TextEncoder` as part of its implementation, but doesn't export one globally. The suggested workaround from the Jest maintainer is to not import `jsdom` reentrantly, and to instead use `jest-environment-node` for tests that import `jsdom` themselves. We don't want to do that; we have some tests that want to import `jsdom` to create a fresh DOM environment to test in isolation with, but some of our transitive dependencies assume the availability of a global `document`/`window` for reasons unimportant to those specific tests. Instead, I worked around this by just re-exporting Node `util`'s `TextEncoder` as `window.TextEncoder` in our unit tests' `jest-setup`. --- #### Jest 28 forbids `describe(SomeThing, ...)` for `SomeThing`s without `name`s In Jest 27, specifying `describe(SomeThing, ...)` when `SomeThing` does not have a name (eg, an arrow function) was not an error; it just produced tests where the "SomeThing" that was intended to be filled in at the beginning of a test name was actually filled in as an empty string. Jest 28 has become more strict about this. Passing a named function or a named class is now explicitly supported, but passing an unnamed thing now produces an error of the form `Invalid first argument, <stringified version of Thing>. It must be a named class, named function, number, or string.` In practice, this error message is very challenging to parse because <stringified version of Thing> is the entire definition of a function, complete with code coverage markers: <details> <summary>Example of one of these errors</summary> ``` FAIL unit tests src/tests/unit/tests/DetailsView/details-view-content.test.tsx ● Test suite failed to run Invalid first argument, function (props) { /* istanbul ignore next */ cov_7cparukpf().f[1]++; var selectedDetailsViewSwitcherNavConfiguration = /* istanbul ignore next */ (cov_7cparukpf().s[20]++, props.deps.getDetailsSwitcherNavConfiguration({ selectedDetailsViewPivot: props.storeState.visualizationStoreData.selectedDetailsViewPivot })); /* istanbul ignore next */ cov_7cparukpf().s[21]++; var renderHeader = function () { /* istanbul ignore next */ cov_7cparukpf().f[2]++; var storeState = /* istanbul ignore next */ (cov_7cparukpf().s[22]++, props.storeState); var visualizationStoreData = /* istanbul ignore next */ (cov_7cparukpf().s[23]++, storeState.visualizationStoreData); /* istanbul ignore next */ cov_7cparukpf().s[24]++; return /*#__PURE__*/React.createElement(_interactiveHeader.InteractiveHeader, { deps: props.deps, selectedPivot: visualizationStoreData.selectedDetailsViewPivot, featureFlagStoreData: storeState.featureFlagStoreData, tabClosed: props.storeState.tabStoreData.isClosed, navMenu: selectedDetailsViewSwitcherNavConfiguration.leftNavHamburgerButton, isSideNavOpen: props.isSideNavOpen, setSideNavOpen: props.setSideNavOpen, narrowModeStatus: props.narrowModeStatus }); }; /* istanbul ignore next */ cov_7cparukpf().s[25]++; var renderOverlay = function () { /* istanbul ignore next */ cov_7cparukpf().f[3]++; var deps = /* istanbul ignore next */ (cov_7cparukpf().s[26]++, props.deps), storeState = /* istanbul ignore next */ (cov_7cparukpf().s[27]++, props.storeState); /* istanbul ignore next */ cov_7cparukpf().s[28]++; return /*#__PURE__*/React.createElement(_detailsViewOverlay.DetailsViewOverlay, { deps: deps, previewFeatureFlagsHandler: props.deps.previewFeatureFlagsHandler, scopingActionMessageCreator: props.deps.scopingActionMessageCreator, inspectActionMessageCreator: props.deps.inspectActionMessageCreator, detailsViewStoreData: storeState.detailsViewStoreData, scopingStoreData: storeState.scopingPanelStateStoreData, featureFlagStoreData: storeState.featureFlagStoreData, userConfigurationStoreData: storeState.userConfigurationStoreData }); }; /* istanbul ignore next */ cov_7cparukpf().s[29]++; var renderDetailsView = function () { /* istanbul ignore next */ cov_7cparukpf().f[4]++; var deps = /* istanbul ignore next */ (cov_7cparukpf().s[30]++, props.deps), storeState = /* istanbul ignore next */ (cov_7cparukpf().s[31]++, props.storeState); var selectedDetailsRightPanelConfiguration = /* istanbul ignore next */ (cov_7cparukpf().s[32]++, props.deps.getDetailsRightPanelConfiguration({ selectedDetailsViewPivot: storeState.visualizationStoreData.selectedDetailsViewPivot, detailsViewRightContentPanel: storeState.detailsViewStoreData.detailsViewRightContentPanel })); var selectedTest = /* istanbul ignore next */ (cov_7cparukpf().s[33]++, selectedDetailsViewSwitcherNavConfiguration.getSelectedDetailsView(storeState)); var automatedChecksCardsViewData = /* istanbul ignore next */ (cov_7cparukpf().s[34]++, props.deps.getCardViewData(props.storeState.unifiedScanResultStoreData.rules, props.storeState.unifiedScanResultStoreData.results, props.deps.getCardSelectionViewData(props.storeState.cardSelectionStoreData, props.storeState.unifiedScanResultStoreData, props.deps.isResultHighlightUnavailable))); var tabStopRequirementData = /* istanbul ignore next */ (cov_7cparukpf().s[35]++, props.storeState.visualizationScanResultStoreData.tabStops.requirements); var needsReviewCardsViewData = /* istanbul ignore next */ (cov_7cparukpf().s[[36](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:36)]++, props.deps.getCardViewData(props.storeState.needsReviewScanResultStoreData.rules, props.storeState.needsReviewScanResultStoreData.results, props.deps.getCardSelectionViewData(props.storeState.needsReviewCardSelectionStoreData, props.storeState.needsReviewScanResultStoreData, props.deps.isResultHighlightUnavailable))); var targetAppInfo = /* istanbul ignore next */ (cov_7cparukpf().s[[37](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:37)]++, { name: props.storeState.tabStoreData.title, url: props.storeState.tabStoreData.url }); var scanDate = /* istanbul ignore next */ (cov_7cparukpf().s[[38](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:38)]++, props.deps.getDateFromTimestamp(props.storeState.unifiedScanResultStoreData.timestamp)); var scanMetadata = /* istanbul ignore next */ (cov_7cparukpf().s[[39](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:39)]++, { timespan: { scanComplete: scanDate }, targetAppInfo: targetAppInfo, toolData: props.storeState.unifiedScanResultStoreData.toolInfo }); /* istanbul ignore next */ cov_7cparukpf().s[[40](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:40)]++; return /*#__PURE__*/React.createElement(_detailsViewBody.DetailsViewBody, { deps: deps, tabStoreData: storeState.tabStoreData, tabStopsViewStoreData: storeState.tabStopsViewStoreData, assessmentStoreData: storeState.assessmentStoreData, pathSnippetStoreData: storeState.pathSnippetStoreData, featureFlagStoreData: storeState.featureFlagStoreData, selectedTest: selectedTest, detailsViewStoreData: storeState.detailsViewStoreData, visualizationStoreData: storeState.visualizationStoreData, visualizationScanResultData: storeState.visualizationScanResultStoreData, visualizationConfigurationFactory: props.deps.visualizationConfigurationFactory, assessmentsProvider: props.deps.assessmentsProvider, dropdownClickHandler: props.deps.dropdownClickHandler, clickHandlerFactory: props.deps.clickHandlerFactory, assessmentInstanceTableHandler: props.deps.assessmentInstanceTableHandler, issuesTableHandler: props.deps.issuesTableHandler, rightPanelConfiguration: selectedDetailsRightPanelConfiguration, switcherNavConfiguration: selectedDetailsViewSwitcherNavConfiguration, userConfigurationStoreData: storeState.userConfigurationStoreData, automatedChecksCardsViewData: automatedChecksCardsViewData, needsReviewCardsViewData: needsReviewCardsViewData, scanIncompleteWarnings: storeState.unifiedScanResultStoreData.scanIncompleteWarnings, scanMetadata: scanMetadata, isSideNavOpen: props.isSideNavOpen, setSideNavOpen: props.setSideNavOpen, narrowModeStatus: props.narrowModeStatus, tabStopRequirementData: tabStopRequirementData }); }; /* istanbul ignore next */ cov_7cparukpf().s[[41](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:41)]++; return /*#__PURE__*/React.createElement(React.Fragment, null, renderHeader(), renderDetailsView(), renderOverlay()); }. It must be a named class, named function, number, or string. [49](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:49) | import { StoreMocks } from './store-mocks'; [50](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:50) | > [51](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:51) | describe(DetailsViewContent, () => { | ^ [52](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:52) | const pageTitle = 'DetailsViewContainerTest title'; [53](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:53) | const pageUrl = 'http://detailsViewContainerTest/url/'; [54](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:54) | let detailsViewActionMessageCreator: IMock<DetailsViewActionMessageCreator>; at Object.describe (src/tests/unit/tests/DetailsView/details-view-content.test.tsx:51:1) ``` </details> 18 of our test files trigger this issue. In every case, the unnamed component in question is a `NamedFC` React component. The `displayName` that `NamedFC` gives these components is just for the benefit of React debugging messages, and doesn't help Jest - these really are 18 test files with test names like ` renders normally` instead of `ThingUnderTest renders normally`, with an empty string where the component name belongs. I think the ideal way to resolve this would be to completely remove our `NamedFC` wrapper and replace it with `eslint-plugin-react`'s [display-name](https://github.com/jsx-eslint/eslint-plugin-react/blob/HEAD/docs/rules/display-name.md) and/or [function-component-definition](https://github.com/jsx-eslint/eslint-plugin-react/blob/HEAD/docs/rules/function-component-definition.md) rules (with similar settings to [what AirBnB uses as of this issue's resolution](airbnb/javascript#2505)). That would be a huge change, though; we have hundreds of components that use `NamedFC`. I'd want to do a separate PR for that. Instead, this PR just changes the 18 tests in question to use `TheNamedFC.displayName`; this has the advantage that it will break obviously if we ever do the bigger change. --- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Dan Bjorge <[email protected]>

BREAKING CHANGE: Remove the minified UMD build from the package. Minified code is hard to audit and since this is a widely used library it seems more appropriate nowadays to optimize for auditability than to ship a legacy module format that, at best, serves educational purposes nowadays. For production browser use cases, users should be using a bundler. For educational purposes, today's online sandboxes like replit.com offer convenient ways to load npm modules, so the use case for UMD through repos like UNPKG or jsDelivr has largely vanished. Fixes #620

ctavan · 2022-08-05T10:01:21Z

@joepie91 the v9 release of this library will remove the minified UMD build. Please let me know if this addresses your concern.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [uuid](https://github.com/uuidjs/uuid) | dependencies | major | [`^8.3.2` -> `^9.0.0`](https://renovatebot.com/diffs/npm/uuid/8.3.2/9.0.0) | | [@types/uuid](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/uuid) ([source](https://github.com/DefinitelyTyped/DefinitelyTyped)) | dependencies | major | [`^8.3.0` -> `^9.0.0`](https://renovatebot.com/diffs/npm/@types%2fuuid/8.3.0/9.0.1) | --- ### Release Notes <details> <summary>uuidjs/uuid</summary> ### [`v9.0.0`](https://github.com/uuidjs/uuid/blob/HEAD/CHANGELOG.md#900-httpsgithubcomuuidjsuuidcomparev832v900-2022-09-05) [Compare Source](uuidjs/uuid@v8.3.2...v9.0.0) ##### ⚠ BREAKING CHANGES - Drop Node.js 10.x support. This library always aims at supporting one EOLed LTS release which by this time now is 12.x which has reached EOL 30 Apr 2022. - Remove the minified UMD build from the package. Minified code is hard to audit and since this is a widely used library it seems more appropriate nowadays to optimize for auditability than to ship a legacy module format that, at best, serves educational purposes nowadays. For production browser use cases, users should be using a bundler. For educational purposes, today's online sandboxes like replit.com offer convenient ways to load npm modules, so the use case for UMD through repos like UNPKG or jsDelivr has largely vanished. - Drop IE 11 and Safari 10 support. Drop support for browsers that don't correctly implement const/let and default arguments, and no longer transpile the browser build to ES2015. This also removes the fallback on msCrypto instead of the crypto API. Browser tests are run in the first supported version of each supported browser and in the latest (as of this commit) version available on Browserstack. ##### Features - optimize uuid.v1 by 1.3x uuid.v4 by 4.3x (430%) ([#597](uuidjs/uuid#597)) ([3a033f6](uuidjs/uuid@3a033f6)) - remove UMD build ([#645](uuidjs/uuid#645)) ([e948a0f](uuidjs/uuid@e948a0f)), closes [#620](uuidjs/uuid#620) - use native crypto.randomUUID when available ([#600](uuidjs/uuid#600)) ([c9e076c](uuidjs/uuid@c9e076c)) ##### Bug Fixes - add Jest/jsdom compatibility ([#642](uuidjs/uuid#642)) ([16f9c46](uuidjs/uuid@16f9c46)) - change default export to named function ([#545](uuidjs/uuid#545)) ([c57bc5a](uuidjs/uuid@c57bc5a)) - handle error when parameter is not set in v3 and v5 ([#622](uuidjs/uuid#622)) ([fcd7388](uuidjs/uuid@fcd7388)) - run npm audit fix ([#644](uuidjs/uuid#644)) ([04686f5](uuidjs/uuid@04686f5)) - upgrading from uuid3 broken link ([#568](uuidjs/uuid#568)) ([1c849da](uuidjs/uuid@1c849da)) ##### build - drop Node.js 8.x from babel transpile target ([#603](uuidjs/uuid#603)) ([aa11485](uuidjs/uuid@aa11485)) - drop support for legacy browsers (IE11, Safari 10) ([#604](uuidjs/uuid#604)) ([0f433e5](uuidjs/uuid@0f433e5)) - drop node 10.x to upgrade dev dependencies ([#653](uuidjs/uuid#653)) ([28a5712](uuidjs/uuid@28a5712)), closes [#643](uuidjs/uuid#643) ##### [8.3.2](uuidjs/uuid@v8.3.1...v8.3.2) (2020-12-08) ##### Bug Fixes - lazy load getRandomValues ([#537](uuidjs/uuid#537)) ([16c8f6d](uuidjs/uuid@16c8f6d)), closes [#536](uuidjs/uuid#536) ##### [8.3.1](uuidjs/uuid@v8.3.0...v8.3.1) (2020-10-04) ##### Bug Fixes - support expo>=39.0.0 ([#515](uuidjs/uuid#515)) ([c65a0f3](uuidjs/uuid@c65a0f3)), closes [#375](uuidjs/uuid#375) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: Renovate Bot <[email protected]> Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/Droplet/pulls/111 Reviewed-by: Vylpes <[email protected]> Co-authored-by: RenovateBot <[email protected]> Co-committed-by: RenovateBot <[email protected]>

ctavan added this to the 9.0.0 milestone Mar 16, 2022

dbjorge mentioned this issue Apr 26, 2022

chore(deps-dev): bump jest from 27.5.1 to 28.0.0 microsoft/accessibility-insights-web#5421

Merged

ctavan mentioned this issue Aug 3, 2022

feat: remove UMD build #645

Merged

ctavan closed this as completed in #645 Aug 4, 2022

This was referenced Oct 27, 2024

chore(main): release 11.0.0 #808

Merged

chore(main): release 11.0.0 #809

Merged

chore(main): release 11.0.0 #810

Closed

chore(main): release 11.0.0 #813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

npm package contains minified files #620

npm package contains minified files #620

joepie91 commented Mar 15, 2022

broofa commented Mar 15, 2022

joepie91 commented Mar 15, 2022

ctavan commented Mar 16, 2022 •

edited

Loading

ctavan commented Mar 16, 2022

LinusU commented Mar 16, 2022

joepie91 commented Mar 16, 2022

LinusU commented Mar 16, 2022

broofa commented Mar 16, 2022

ctavan commented Mar 16, 2022 •

edited

Loading

ljharb commented Mar 17, 2022

ctavan commented Mar 17, 2022

ljharb commented Mar 17, 2022

ctavan commented Aug 5, 2022

npm package contains minified files #620

npm package contains minified files #620

Comments

joepie91 commented Mar 15, 2022

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Describe alternatives you've considered

Additional context

broofa commented Mar 15, 2022

joepie91 commented Mar 15, 2022

ctavan commented Mar 16, 2022 • edited Loading

ctavan commented Mar 16, 2022

LinusU commented Mar 16, 2022

joepie91 commented Mar 16, 2022

LinusU commented Mar 16, 2022

broofa commented Mar 16, 2022

ctavan commented Mar 16, 2022 • edited Loading

ljharb commented Mar 17, 2022

ctavan commented Mar 17, 2022

ljharb commented Mar 17, 2022

ctavan commented Aug 5, 2022

ctavan commented Mar 16, 2022 •

edited

Loading

ctavan commented Mar 16, 2022 •

edited

Loading