feat: make require.resolve respect ignoreDynamicRequires
#1530
Conversation
liuxingbaoyu
requested review from
tada5hi,
tjenkinson and
shellscape
as code owners
| @@ -30,7 +30,7 @@ | |||
| "ci:lint:commits": "commitlint --from=${CIRCLE_BRANCH} --to=${CIRCLE_SHA1}", | |||
| "ci:test": "pnpm test -- --verbose", | |||
| "prebuild": "del-cli dist", | |||
| "prepare": "if [ ! -d 'dist' ]; then pnpm build; fi", | |||
| "prepare": "node -e \"fs.existsSync('./dist') || process.exit(1)\" || pnpm build", | |||
There was a problem hiding this comment.
This is a great idea to make it platform independent, but this will mess up release as all plugins will get a new release with a confusing error message. This should be put into a separate PR.
| }␊ | ||
| handleRequire.resolve = function (path) {␊ | ||
| var resolvedPath = commonjsResolve(path, originalModuleDir);␊ | ||
| if (resolvedPath !== null) {␊ | ||
| return resolvedPath;␊ | ||
| }␊ | ||
| return require.resolve(path);␊ | ||
| throw new Error('Could not dynamically require/require.resolve "' + path + '". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.');␊ |
There was a problem hiding this comment.
IMO it is unfortunate to repeat this huge error message twice in the user's code. And if we do, then we do not need the require/require.resolve duality in the message because we can use two separate messages. But I would prefer to find a way to not repeat it (and possibly shorten it if this is possible without loss in information).
| @@ -116,7 +119,7 @@ export function ${CREATE_COMMONJS_REQUIRE_EXPORT}(originalModuleDir) { | |||
| if (resolvedPath !== null) { | |||
| return resolvedPath; | |||
| } | |||
| return require.resolve(path); | |||
| ${ignoreDynamicRequires ? 'return require.resolve(path);' : FAILED_REQUIRE_ERROR} | |||
There was a problem hiding this comment.
This makes it a breaking change. I.e. where before we had a fallback to require.resolve, we now need to potentially configure more require targets. Which is fine, but we need to take care to make this a major release.
|
@liuxingbaoyu please see the previous review. If this has no action after 60 days we'll consider it abandoned. |
Rollup Plugin Name: @rollup/plugin-commonjs
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
(I'm not sure if this is a breaking change, since it seems unnecessary to allow
require.resolvewhen disallowingrequire)List any relevant issue numbers:
Fixes #1472
Fixes babel/babel#14301
Description
PR consists of two parts
The first part is to allow the project to run
pnpm buildon Windows.The second part is to make
require.resolverespectignoreDynamicRequires.