fix: remove isolated resolve() for compat with browser distribution

[cmorten]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers: #3934

Description

The use of resolve() here is incompatible with the implementation of the method for browser usage of Rollup, see https://github.com/rollup/rollup/blob/master/browser/path.ts#L60, specifically the code:

export function resolve(...paths: string[]) {
	let resolvedParts = paths.shift()!.split(/[/\\]/);

        // ... rest of function
}

where without at least one argument, this function will throw an Uncaught TypeError when trying to call .split() on paths.shift() which will be undefined as paths is an empty array.

Given the NodeJS implementation of resolve(), which will always fallback to prepending the CWD if unable to construct an absolute path, this PR's refactor is entirely equivalent for NodeJS while also supporting the bespoke implementation for the browser distribution. In essense, for NodeJS resolve(resolve(), source) === resolve(source).

NodeJS docs: https://nodejs.org/api/path.html#path_path_resolve_paths

I'm struggling to find any browser specific tests to cover this use-case, please advise if this is something that is required - would appreciate some guidance where to add!

In a way this side-steps the issue, which is the browser implementation of resolve() does not quite mirror the NodeJS one. For that it would need to be able to handle no arguments, but then the expected behaviour would be to return the CWD which doesn't make much sense in the browser context... but perhaps it could at least not throw with something like:

let resolvedParts = (paths.shift() || "").split(/[/\\]/);

regardless, it's implementation detail becomes moot if that use is never invoked.

[codecov]

Codecov Report

Merging #3935 (37bdbee) into master (4519b11) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3935   +/-   ##
=======================================
  Coverage   97.18%   97.18%           
=======================================
  Files         188      188           
  Lines        6681     6681           
  Branches     1944     1944           
=======================================
  Hits         6493     6493           
  Misses         99       99           
  Partials       89       89           

Impacted Files	Coverage Δ
browser/path.ts	76.92% <ø> (ø)
src/utils/relativeId.ts	100.00% <100.00%> (ø)
src/utils/resolveId.ts	94.73% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4519b11...37bdbee. Read the comment docs.

[lukastaegert]

Thanks for tackling this! I understand that core of the problem is that we do not have tests for functionality specific to the browser bundle. It might be possible to set something up using the ESM version in Node 14, though. I will have a look at this in the next days and possibly push something to your branch if you do not mind.

[cmorten]

Thanks for tackling this! I understand that core of the problem is that we do not have tests for functionality specific to the browser bundle. It might be possible to set something up using the ESM version in Node 14, though. I will have a look at this in the next days and possibly push something to your branch if you do not mind.

Sounds perfect!

[lukastaegert]

So I ended up pushing quite a bit more to your branch, including a full test-suite for the browser bundle (though I ended up testing the UMD bundle instead of the ESM bundle).
Only thing I could not make to work satisfactorily is merging coverage between the two test runs. I assume since one is testing the browser bundle that is very different from the commonjs bundle the other is testing, sourcemap positions do not always match and the merged coverage was always much lower than the individual coverage.
But you can check coverage for the browser tests via npm run test:coverage:browser.

Also, regular npm test is now slower because it is doing the full build, not only the CommonJS build. To get the old performance, run npm run test:cjs.

From the content, I kept your change because it looks to me like an overall performance improvement, but ended up entirely replacing the resolveId algorithm for the browser, as everything else in that function also would not apply here as we have no disk access and e.g. cannot "try" different extensions as that again relies on file system operations.

[cmorten]

All sounds very sensible - thanks a tonne @lukastaegert for putting down all the effort, introducing a new test suite etc. on my behalf! Quite happy being a passenger in my own PR 😋 I'll be sure to follow precedent and extend the new brower suite in any future contributions (if appropriate).