fix(build): remove warning about ineffective dynamic import from node_modules

[chaejunlee]

Description

fixes #13848.

I have added a BuildOption variable called warnExternalChunkRender.

Default is false which disables warning of ineffective dynamic imports of external modules.

Setting the value to true enables all ineffective dynamic import warnings.

Additional context

I not so sure if I added the variable in the right place, or with a right name.

If you give me any feedback about that, I will fix it right away.

If the members approve this, I will be responsible for adding the documentation for this new config variable.

I would like to add some robust testing around this, but I really can't do it because I can't add node_modules to the codebase. I checked that the playground/dynamic-import has some package.json hack, but the build still targets the user defined files (pkg) not node_modules. If you can guide a little more on this, I will make sure to add testing as well.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[andygup]

@chaejunlee I'm not familiar with the build/test processes of this repo, but I'm guessing you will need to modify the existing spec, or perhaps create a one to handle the new use case. Did you take a look at the spec in the original PR?

[chaejunlee]

@andygup I guess you are mentioning the tests. I didn't through the original PR but I did look into it while preparing this PR. I will go through the original PR again and try again with the test. Thank you!

[DesignHhuang]

i approved these changes that can enhance the build options

[chaejunlee]

I have added documentation for the new config option. To better explain its functionality, I coined the term 'Ineffective Dynamic Import' as mentioned in the original PR #12850.

Feedback is welcome!

[DesignHhuang]

I think this is a good idea. I am waiting for these changes to be merged. Then i can use this configuration instead of writing code to monitor these warns with build.rollupOptions.onwarn .

[DesignHhuang]

well done

[chaejunlee]

@bluwy Thank you so much for the review!

What I understood is that don't make this feature configurable as a build option, but still implement the reporter.ts part as a feature.

Correct me if I am wrong. Until then, I will be removing config bits and refactor the reporter.ts as you pointed out.

[bluwy]

Yup, I think we can avoid the option for now, and only implement the fix to not emit warnings for modules that contain "node_modules"

[bluwy]

Looks great! Gonna wait for a bit if another maintainer has thoughts on this (and the option) before merging.

[andygup]

Nice job @chaejunlee!

only implement the fix to not emit warnings for modules that contain "node_modules"

@bluwy thanks, this fix will significantly reduce the number of warnings for our library and possibly others. We use lazy loading internally in various @arcgis/core modules to avoid circular dependencies, so it will be nice to not have to instruct end users to suppress all warnings.

[davidAtInleague]

It looks like warnExternalChunkRender was planned to silence emitting these errors from user code, but was removed in favor of silencing only errors originating from node_modules. Is there a mechanism to silence these errors when they originate in user code?

I see 7a77aaf, but it requires enabling inlineDynamicImports, which it's not clear I otherwise want to do.

[andygup]

@davidAtInleague maybe at the bundler level, this was a suggestion provided in my original issue: https://rollupjs.org/configuration-options/#onwarn

[davidAtInleague]

@andygup thank you so much! That is the solution.