From 40eee93e6b30f41d1cd334c53b2b6c456875e57b Mon Sep 17 00:00:00 2001 From: Tom Jenkinson Date: Sat, 28 Nov 2020 02:33:10 +0000 Subject: [PATCH] feat(node-resolve)!: Mark built-ins as external (#627) * do not ignore errors * do not error when import specifier does not map to file * always swallow MODULE_NOT_FOUND errors as not being able to resolve the id is a valid output * return null from resolveId early if can't resolve * strip null byte from importer if there is one * fix root-dir test this was passing before because the real error was being ignored * remove try/catch * treat builtins as externals (depending on useBuiltins option) * update readme sections on builtins * fix readme doc about `jail` * update all other snapshots * use exists() instead of try/catch * refactor resolveImportSpecifiers to use async/await * use async `realpath` * remove duplicate lines * remove readme update * fix doc * add note that `preferBuiltins` should be `false` for stubbed builtins --- packages/node-resolve/README.md | 18 +++------- packages/node-resolve/src/index.js | 4 +-- .../src/resolveImportSpecifiers.js | 26 +++++--------- packages/node-resolve/test/prefer-builtins.js | 8 ++--- .../test/snapshots/dedupe-custom.js.md | 2 +- .../node-resolve/test/snapshots/dedupe.js.md | 2 +- .../test/snapshots/dedupe.js.snap | Bin 354 -> 356 bytes .../node-resolve/test/snapshots/jail.js.md | 2 +- .../node-resolve/test/snapshots/only.js.md | 8 ++--- .../node-resolve/test/snapshots/only.js.snap | Bin 435 -> 434 bytes .../test/snapshots/prefer-builtins.js.md | 32 +----------------- .../test/snapshots/prefer-builtins.js.snap | Bin 629 -> 308 bytes .../test/snapshots/root-dir.js.md | 8 +---- .../test/snapshots/root-dir.js.snap | Bin 169 -> 143 bytes .../node-resolve/test/snapshots/test.js.md | 2 +- .../node-resolve/test/snapshots/test.js.snap | Bin 618 -> 618 bytes 16 files changed, 28 insertions(+), 84 deletions(-) diff --git a/packages/node-resolve/README.md b/packages/node-resolve/README.md index 0f49f6d70..ade673afe 100755 --- a/packages/node-resolve/README.md +++ b/packages/node-resolve/README.md @@ -112,7 +112,7 @@ Specifies the extensions of files that the plugin will operate on. Type: `String`
Default: `'/'` -Locks the module search within specified path (e.g. chroot). Modules defined outside this path will be marked as external. +Locks the module search within specified path (e.g. chroot). Modules defined outside this path will be ignored by this plugin. ### `mainFields` @@ -129,7 +129,6 @@ DEPRECATED: use "resolveOnly" instead ### `preferBuiltins` Type: `Boolean`
-Default: `true` If `true`, the plugin will prefer built-in modules (e.g. `fs`, `path`). If `false`, the plugin will look for locally installed modules of the same name. @@ -183,20 +182,11 @@ export default { ## Resolving Built-Ins (like `fs`) -This plugin won't resolve any builtins (e.g. `fs`). If you need to resolve builtins you can install local modules and set `preferBuiltins` to `false`, or install a plugin like [rollup-plugin-node-polyfills](https://github.com/ionic-team/rollup-plugin-node-polyfills) which provides stubbed versions of these methods. +By default this plugin will prefer built-ins over local modules, marking them as external. -If you want to silence warnings about builtins, you can add the list of builtins to the `externals` option; like so: +See [`preferBuiltins`](#preferbuiltins). -```js -import resolve from '@rollup/plugin-node-resolve'; -import builtins from 'builtin-modules' -export default ({ - input: ..., - plugins: [resolve()], - external: builtins, - output: ... -}) -``` +Use a plugin like [rollup-plugin-node-polyfills](https://github.com/ionic-team/rollup-plugin-node-polyfills) to provide stubbed versions of built-ins, and set `preferBuiltins` to `false`. ## Resolving require statements diff --git a/packages/node-resolve/src/index.js b/packages/node-resolve/src/index.js index 1d5d67fb9..858d47eda 100644 --- a/packages/node-resolve/src/index.js +++ b/packages/node-resolve/src/index.js @@ -259,14 +259,14 @@ export function nodeResolve(opts = {}) { if (hasPackageEntry) { if (builtins.has(resolved) && preferBuiltins && isPreferBuiltinsSet) { - return null; + return false; } else if (importeeIsBuiltin && preferBuiltins) { if (!isPreferBuiltinsSet) { this.warn( `preferring built-in module '${importee}' over local alternative at '${resolved}', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning` ); } - return null; + return false; } else if (jail && resolved.indexOf(normalize(jail.trim(sep))) !== 0) { return null; } diff --git a/packages/node-resolve/src/resolveImportSpecifiers.js b/packages/node-resolve/src/resolveImportSpecifiers.js index 5f780967e..f3bd03f9f 100644 --- a/packages/node-resolve/src/resolveImportSpecifiers.js +++ b/packages/node-resolve/src/resolveImportSpecifiers.js @@ -160,38 +160,30 @@ async function resolveId(importPath, options, exportConditions, warn) { // Resolve module specifiers in order. Promise resolves to the first module that resolves // successfully, or the error that resulted from the last attempted module resolution. -export function resolveImportSpecifiers( +export async function resolveImportSpecifiers( importSpecifierList, resolveOptions, exportConditions, warn ) { - let promise = Promise.resolve(); - for (let i = 0; i < importSpecifierList.length; i++) { - // eslint-disable-next-line no-loop-func - promise = promise.then(async (value) => { - // if we've already resolved to something, just return it. - if (value) { - return value; - } - + try { + // eslint-disable-next-line no-await-in-loop let result = await resolveId(importSpecifierList[i], resolveOptions, exportConditions, warn); if (!resolveOptions.preserveSymlinks) { + // eslint-disable-next-line no-await-in-loop if (await exists(result)) { + // eslint-disable-next-line no-await-in-loop result = await realpath(result); } } return result; - }); - - // swallow MODULE_NOT_FOUND errors - promise = promise.catch((error) => { + } catch (error) { + // swallow MODULE_NOT_FOUND errors if (error.code !== 'MODULE_NOT_FOUND') { throw error; } - }); + } } - - return promise; + return null; } diff --git a/packages/node-resolve/test/prefer-builtins.js b/packages/node-resolve/test/prefer-builtins.js index dda3f5008..977d4e789 100644 --- a/packages/node-resolve/test/prefer-builtins.js +++ b/packages/node-resolve/test/prefer-builtins.js @@ -9,7 +9,7 @@ const { nodeResolve } = require('..'); process.chdir(join(__dirname, 'fixtures')); -test('warns when importing builtins', async (t) => { +test('handles importing builtins', async (t) => { const warnings = []; const bundle = await rollup({ input: 'builtins.js', @@ -24,8 +24,7 @@ test('warns when importing builtins', async (t) => { const { module } = await testBundle(t, bundle); - t.is(warnings.length, 1); - t.snapshot(warnings); + t.is(warnings.length, 0); // eslint-disable-next-line global-require t.is(module.exports, require('path').sep); }); @@ -66,8 +65,7 @@ test('true allows preferring a builtin to a local module of the same name', asyn const imports = await getImports(bundle); - t.is(warnings.length, 1); - t.snapshot(warnings); + t.is(warnings.length, 0); t.deepEqual(imports, ['events']); }); diff --git a/packages/node-resolve/test/snapshots/dedupe-custom.js.md b/packages/node-resolve/test/snapshots/dedupe-custom.js.md index 98f8f1114..5465a0577 100644 --- a/packages/node-resolve/test/snapshots/dedupe-custom.js.md +++ b/packages/node-resolve/test/snapshots/dedupe-custom.js.md @@ -2,7 +2,7 @@ The actual snapshot is saved in `dedupe-custom.js.snap`. -Generated by [AVA](https://ava.li). +Generated by [AVA](https://avajs.dev). ## can deduplicate custom module directory diff --git a/packages/node-resolve/test/snapshots/dedupe.js.md b/packages/node-resolve/test/snapshots/dedupe.js.md index 62b1a6ca6..5e20bc021 100644 --- a/packages/node-resolve/test/snapshots/dedupe.js.md +++ b/packages/node-resolve/test/snapshots/dedupe.js.md @@ -2,7 +2,7 @@ The actual snapshot is saved in `dedupe.js.snap`. -Generated by [AVA](https://ava.li). +Generated by [AVA](https://avajs.dev). ## dedupes deep imports by package name if dedupe is set diff --git a/packages/node-resolve/test/snapshots/dedupe.js.snap b/packages/node-resolve/test/snapshots/dedupe.js.snap index e91f821dc56a81106e1c33bf00e70c36b552dfcc..f03048c2e723b83356d6f6b9e9906f4f38f7c497 100644 GIT binary patch delta 305 zcmV-10nYy70^|ZBK~_N^Q*L2!b7*gLAa*he0sus{jH)LwpzK)Ly#vn>0)CMqK7YZk zCI4neU74`@X`Ic=F@i-Um>3vD8H@!FDooFftnfX$v6E0AgWAHbF)<|D>$c0mMbd#aNR+z?Mb8Z5q&gBBzL%cI3u{jf`^9o6K9&d1RenD!AOKM6f zkb;Q{7lTE0Qy{|F+$)Nr5#8O|s0JWBfW_b;7|?Wgo)lK+C1+&jqyPXZg9aC3-va;u DR(gnU delta 303 zcmV+~0nq;B0^$N9K~_N^Q*L2!b7*gLAa*he0sz18V&MGEeq+|KH^18X@sW`tK7WB| z!;<@{DU0ShycST~_aQEp5iI%^h<7ddH#_Regw;>uY+f#3QhARNEV`YEfg$F?Ebk@p zF{LL9_A35l5pHJ$i-t2ZFfcQ)gY9JGU}QD|(iT9R0mQ$t8@8aHfEO z00Sd010xdyBR^0LlL#X-10$;-BY$g9YGQH;j3-nC;_0WPrWWXB6Qth diff --git a/packages/node-resolve/test/snapshots/jail.js.md b/packages/node-resolve/test/snapshots/jail.js.md index ff9eee7dc..b9ee3395b 100644 --- a/packages/node-resolve/test/snapshots/jail.js.md +++ b/packages/node-resolve/test/snapshots/jail.js.md @@ -2,7 +2,7 @@ The actual snapshot is saved in `jail.js.snap`. -Generated by [AVA](https://ava.li). +Generated by [AVA](https://avajs.dev). ## mark module outside the jail as external diff --git a/packages/node-resolve/test/snapshots/only.js.md b/packages/node-resolve/test/snapshots/only.js.md index 0996c4937..16eb1a462 100644 --- a/packages/node-resolve/test/snapshots/only.js.md +++ b/packages/node-resolve/test/snapshots/only.js.md @@ -2,7 +2,7 @@ The actual snapshot is saved in `only.js.snap`. -Generated by [AVA](https://ava.li). +Generated by [AVA](https://avajs.dev). ## deprecated: regex @@ -30,19 +30,19 @@ Generated by [AVA](https://ava.li). }, ] -## regex +## handles nested entry modules > Snapshot 1 [] -## specify the only packages to resolve +## regex > Snapshot 1 [] -## handles nested entry modules +## specify the only packages to resolve > Snapshot 1 diff --git a/packages/node-resolve/test/snapshots/only.js.snap b/packages/node-resolve/test/snapshots/only.js.snap index 4f4bed592576a826608a857c63089d2c1cef9638..dec35a9b170722262304fc4480799be14c29a06a 100644 GIT binary patch delta 388 zcmV-~0ek+l1F{1mK~_N^Q*L2!b7*gLAa*he0sv6p{Ec-_8X(D%CHne$)ryfKC@bdW z|7flUuZ=Y=AH`Rzv~e+lMW+L?|Ed<|=i-gslh>?Wl>c!KPzEHr3z1(HfA0dZq+<-5 zNZ-6SFQ>E5dV78fCnH!ikePvjnSmW_CnEu$g i8RQ5?W+VQKLaBd1AikU10y#hqYX?GYd(-J1>yxj%mK8{m=S6Kn}1SP zYH|r96Pzg^Ai%)L%fQIOz{n3&!z9AU%)rPh$jFkMpOOmW@CEpUx_kP?hdT!OdHT7d zDP_-1EiO(>hpYC;18UVRN-fUMDND6d2+2rQNXXC2sZ3DFFDS{(&ns5QENfOsNi8T! zO-?LHP0>*($Vp8sPE{xc;smHRe~@}Kd)NwcO4Bp*U|b$#YZw^$7#V$m-h|0=l;j7O z6lLb6GcZbm3=xC`0Eb&?UNXoLjLb+JU?3pE9A{9Iq+~Sa22sob0G#bDQ`rLm01;%Y AasU7T diff --git a/packages/node-resolve/test/snapshots/prefer-builtins.js.md b/packages/node-resolve/test/snapshots/prefer-builtins.js.md index 4653621e7..75d479022 100644 --- a/packages/node-resolve/test/snapshots/prefer-builtins.js.md +++ b/packages/node-resolve/test/snapshots/prefer-builtins.js.md @@ -2,7 +2,7 @@ The actual snapshot is saved in `prefer-builtins.js.snap`. -Generated by [AVA](https://ava.li). +Generated by [AVA](https://avajs.dev). ## false allows resolving a local module with the same name as a builtin module @@ -16,33 +16,3 @@ Generated by [AVA](https://ava.li). toString: Function {}, }, ] - -## true allows preferring a builtin to a local module of the same name - -> Snapshot 1 - - [ - { - code: 'UNRESOLVED_IMPORT', - importer: 'prefer-builtin.js', - message: '\'events\' is imported by prefer-builtin.js, but could not be resolved – treating it as an external dependency', - source: 'events', - toString: Function {}, - url: 'https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency', - }, - ] - -## warns when importing builtins - -> Snapshot 1 - - [ - { - code: 'UNRESOLVED_IMPORT', - importer: 'builtins.js', - message: '\'path\' is imported by builtins.js, but could not be resolved – treating it as an external dependency', - source: 'path', - toString: Function {}, - url: 'https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency', - }, - ] diff --git a/packages/node-resolve/test/snapshots/prefer-builtins.js.snap b/packages/node-resolve/test/snapshots/prefer-builtins.js.snap index 509265fa31fd89b309629217ba5dd0ea989c1d2e..a68c04dd6996992cfa42761ae165ca333ba2632d 100644 GIT binary patch literal 308 zcmV-40n7eDRzVU(cj+wF)^ibHfUXGId;F6-uymSUeNst|a5MOe*mF6XvWaj5FG9z(-J_P_X7GKSI G0RRB)V0tzH literal 629 zcmV-*0*d`XRzVjQe7*4_?H`K>00000000Bk zQ_pJ?K@^^s%`aPR66!%c4ecQy-3AI?f){P02u)I&R8VN@W_OZovYA+ zn|LaCQSewn6#N53@FWNx^yWzqqE~0P*(FWI9E5-iAM<8-KfZn6dsT#xf(}<-{^(cV zHu9C^^KbaYPe9(6k04aLyZ!m-d%v^z>lLd#*#8Rt>w$vM;N!j6>YM#bk83l{_lH-G zz<+%m^YSJ9ct}EINuDbb^AOKK>>B4<4Rop0V zR>!=PX4ht15FSR@oHPhm>#k`DlPmAVL2N<|LiOGlo4CVV zu7_me$6OH10nAI;!rv6EBDl;w7OuG53hZJ_2;0f$a-3O~YqyL|~PUZKpfe90Rw)rH9@C?gjs;lEB!jLFlLcp~vm;=<&K ziHY?r#%zwou%t-r2dnRa#pwOc<5RHwh(N^X9`7w`+?Y_G{u`by3Yo8nAf#zxlGMC;TljwG1E5=K1^zJSXA Pu!6q<)r{x55e5JN7KSf$ diff --git a/packages/node-resolve/test/snapshots/root-dir.js.md b/packages/node-resolve/test/snapshots/root-dir.js.md index 004f7d9b6..64b00a4b4 100644 --- a/packages/node-resolve/test/snapshots/root-dir.js.md +++ b/packages/node-resolve/test/snapshots/root-dir.js.md @@ -2,13 +2,7 @@ The actual snapshot is saved in `root-dir.js.snap`. -Generated by [AVA](https://ava.li). - -## deduplicated from the given root directory - -> Snapshot 1 - - 'Package A React: react imported from root | package B react: react imported from root' +Generated by [AVA](https://avajs.dev). ## deduplicates modules from the given root directory diff --git a/packages/node-resolve/test/snapshots/root-dir.js.snap b/packages/node-resolve/test/snapshots/root-dir.js.snap index 0dfe37a480a51a7cc3d7e115ef5d612834666e0b..98b3cdf3b0adf2ca8c0a2ef47ccfc01a5551a418 100644 GIT binary patch literal 143 zcmZ<^b5sb`nVidcAiuX<8oE- tQZ(MKuz!V$sjK2z4g2tg8ut5L7P>8DSapqa5y$o@hGSx~Q<{Oc001H>Hf#U@ literal 169 zcmV;a09OA&RzVbfYzzWO@)E$u`C4WKC z3M87KrXZN2%bgQ0a_r#q(L@yz3t~WGpaV0A4Iw6mGE}fIF*7iLR0IP%JMWVClqN!) z^y!}8=l8wO&)y=0Sj0YcZ?ExIfBNy^%*)-z=RT15tELF~zO(Z9_HXj-;B;Nzd1n2B zVfHEF$;pq;*ZG(9QuOB7yS?fT41crltynKRtcM(=R`Fj!9EVI1(wCHS2LL9@=ko*> z2mpaafQzOmyN&+`h}Vdpag2UmA@*S`l&Lj4%+ruc!a|`y;3$C%fjsU>odAnKx(IWg z^qE-{Z&o+g>b2{))~+K%hnkkc5_8IHhqpU5Yu)ETaIDad5!l}w^ zmxZC*PFnR#Eh(M2xf;bC4drMSi)#o*9z6A1guDL6K~wr zx))w>93?rAdY!N=RoiJtfq%~&CY+URR|!->cnfDzZ$=4M1XjNc!AhQ zd`0|06yQHEHAmn`Y&sd+f*aHEsm_imfLZRG;Zn2QdBa(TTO4r#jwh*a$hoAj43$k3ht#m!Uo7d@EQkS#fey?dHiVcM%22_=#LU0|QV|U7?7U0jQ<|va zq)&eLKELmM&-NxE#3Dbom!91DL%#2yuIanat=~Z6?-Ig3b$_@1PJjOS@yx58`j;LI zvd>b4JU#j8#cJ?1y%4=U_I`JL8-E7b4_2&~9n?db6rplOMuA6#)Ku|M1`jwbm7S)P zCE8T7LnARtdv+jQ7L?mzQlEp=GOiWGamW-QeOW2i2QX1SpC>R!00_(jTr@@5EnFWW z-XMO(G0yWEu?HieOtsPGu7*?+777IdM+syI> zUb$MozIL;^u{p6j=XbhNX|7CS(c`>p53{zA!tO{f3OEiqQJL-VFl4QyRj-6cW6fs>(NG^9xAy^89E<7L)!7jRFwLDaTxyy-Z#c_v^Fz+ZF(-S5oJk6sEC~65 ghH1miY84%%GE-E=P}xLrKn=S61FJt$z77Nc0C;H}?EnA(