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
fix: remove isolated resolve() for compat with browser distribution #3935
fix: remove isolated resolve() for compat with browser distribution #3935
Conversation
This is incompatible with the browser implementation of the method.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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! |
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). Also, regular 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. |
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). |
Co-authored-by: Craig Morten <cmorten@users.noreply.github.com>
3adbedd
to
4770ed5
Compare
This PR contains:
Are tests included?
Breaking Changes?
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:where without at least one argument, this function will throw an
Uncaught TypeError
when trying to call.split()
onpaths.shift()
which will beundefined
aspaths
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 NodeJSresolve(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:regardless, it's implementation detail becomes moot if that use is never invoked.