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

Option to disable CommonJS/AMD warnings. #25784

Closed
fudom opened this issue Sep 5, 2023 · 8 comments · Fixed by #26047
Closed

Option to disable CommonJS/AMD warnings. #25784

fudom opened this issue Sep 5, 2023 · 8 comments · Fixed by #26047

Comments

@fudom
Copy link

fudom commented Sep 5, 2023

Which @angular/* package(s) are relevant/related to the feature request?

compiler

Description

In a real world applications it's impossible to 100% avoid CommonJS/AMD dependencies. Either because of need or indirect dependencies which cannot be changed.

Docs

Proposed solution

Instead of filling the allowedCommonJsDependencies array, add an option to disable all warnings.

Alternatives considered

Or allow wildcard * to allow all.

@JeanMeche JeanMeche transferred this issue from angular/angular Sep 5, 2023
@alan-agius4
Copy link
Collaborator

In a real world applications it's impossible to 100% avoid CommonJS dependencies

I don't really agree with this statement. Can you provide some explains of which libraries you are having trouble with? In general CJS libraries are not meant to be used in the browser.

@fudom
Copy link
Author

fudom commented Sep 6, 2023

I mean packages that are either CJS/AMD themselves or indirectly use CJS/AMD dependencies.
Like @ionic/storage, @turf, keycloak-js, dayjs, semver, etc.

Warning: foo.ts depends on 'dayjs'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: foo.ts depends on 'dayjs/plugin/relativeTime'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: foo.ts depends on 'dayjs/locale/foo'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: foo.ts depends on 'fast-xml-parser'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: foo.ts depends on 'jszip'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: foo.ts depends on 'ajv'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: foo.ts depends on 'markdown-it-header-sections'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: node_modules\keycloak-js\dist\keycloak.mjs depends on 'js-sha256'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: node_modules\keycloak-js\dist\keycloak.mjs depends on 'base64-js'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: node_modules\@turf\line-overlap\dist\es\index.js depends on 'deep-equal'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: node_modules\@turf\boolean-overlap\dist\es\index.js depends on 'geojson-equality'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: node_modules\@turf\buffer\dist\es\index.js depends on 'turf-jsts'. CommonJS or AMD dependencies can cause optimization bailouts.

Warning: node_modules\@ionic\storage\__ivy_ngcc__\fesm2015\ionic-storage.js depends on 'localforage'. CommonJS or AMD dependencies can cause optimization bailouts.

...

Some deps could be replaced with alternatives, others not. Dependencies must be chosen carefully. CJS/AMD warnings are unavoidable when using required packages. Directly or indirectly. It's utopian and wishful thinking to have only clean ESM in a complex real real world application. Finally, the developers should decide to generally allow such packages without having to maintain the array list. For example by adding "*" or separate property to suppress the warnings.

@fudom fudom changed the title Disable CommonJS all warnings. Disable CommonJS/AMD all warnings. Sep 6, 2023
@fudom fudom changed the title Disable CommonJS/AMD all warnings. Disable all CommonJS/AMD warnings. Sep 6, 2023
@angular-robot angular-robot bot added the feature: votes required Feature request which is currently still in the voting phase label Sep 7, 2023
@angular-robot
Copy link
Contributor

angular-robot bot commented Sep 7, 2023

This feature request is now candidate for our backlog! In the next phase, the community has 60 days to upvote. If the request receives more than 20 upvotes, we'll move it to our consideration list.

You can find more details about the feature request process in our documentation.

@twoco
Copy link

twoco commented Oct 1, 2023

I have a similar issue with that. In a larger project (monorepo) you cannot avoid CJS/AMD dependencies. There is a PR (#25931) which cannot be merged because of Contributor License Agreement (CLA). I'm ok with the CLA. But I don't have and don't want a Google account. So you can just do it ... Here's is my proposal. It's really simple and does not have any critical breaking impact.

Just add allowedRequests.has('*') to

if (allowedRequests.has(request)) {
notAllowed = false;
} else {

if (allowedRequests.has('*') || allowedRequests.has(request)) {
  notAllowed = false;
} // else ... 

@fudom
Copy link
Author

fudom commented Oct 9, 2023

Not sure if any issue can reach 20 upvotes in this project (cli). Maybe with bots. It's like an indirect close. I understand the idea behind. But in this case, why not just implement this simple change regardless of this? With minimal effort and a powerful option that allows the developer to decide what to allow and what not. The developer would add a long list of all the packages anyway. It's just more effort, uglier and repeating task. Using wildcard in such cases isn't unusual. Why blocking this? Any concerns? Ignoring is just disrespectful.

@alan-agius4
Copy link
Collaborator

PRs are welcome add wildcard support.

@angular angular deleted a comment from angular-robot bot Oct 16, 2023
@fudom fudom changed the title Disable all CommonJS/AMD warnings. Option to disable CommonJS/AMD warnings. Oct 17, 2023
@angular-robot
Copy link
Contributor

angular-robot bot commented Oct 17, 2023

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

domske added a commit to domske/angular-cli that referenced this issue Oct 17, 2023
…CommonJsDependencies`

This commit adds the functionality to that when a wildcard `*` is provided to `allowedCommonJsDependencies` CJS/AMD warnings usages is skipped.

Closes angular#25784
domske added a commit to domske/angular-cli that referenced this issue Oct 18, 2023
…CommonJsDependencies`

This commit adds the functionality to that when a wildcard `*` is provided to `allowedCommonJsDependencies` CJS/AMD warnings usages is skipped.

Closes angular#25784
dgp1130 pushed a commit that referenced this issue Oct 19, 2023
…CommonJsDependencies`

This commit adds the functionality to that when a wildcard `*` is provided to `allowedCommonJsDependencies` CJS/AMD warnings usages is skipped.

Closes #25784
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants