fix(alias): paths for aliases across multiple Windows drives

[Robinfr]

Rollup Plugin Name: alias

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

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

We ran into an issue where we had aliases to paths on another Windows drive which was not working. After some investigation, I found out that this is because the drive letter is being removed in the alias plugin. Removing that piece of code made the alias work properly across multiple Windows drives.

Questions:

• I also removed usage of the slash library as it's not actually needed to work properly. Do you want me to remove the library entirely?
• The normalize functions are doing nothing more than just returning the id and path straight away, so they could technically be removed entirely. Do you want me to take care of that?

[Robinfr]

Can I poke someone for this? We're already using this fix ourselves using patch-package but would be nice to get this also i the official repo.

[shellscape]

Could you share some more information about the issue you ran into? As much detail as possible would be helpful. This is a rather impactful change so we need to fully understand what was happening.

[Robinfr]

Sure, here's the situation:

We have some common dependencies installed on drive C:/, but the project where Rollup is being used in on drive D:/. We are using the alias plugin to alias some of these common dependencies, e.g.:

"react" -> "C:/common/node_modules/react"

However, when it tries to resolve the import in a file on the D:/ project that looks like this:

import React from "react";

The resolve fails because the id is incorrect as it removes the volume from the path, e.g. it becomes /common/node_modules/react which is incorrect. Rollup however does not remove the volume when generating the bundle, so there is a mismatch of ids when trying to resolve the id (I believe).

Removing the normalization as it is implemented in this merge request, fixes this issue by keeping the volume letter.

[shellscape]

putting dependencies in a separate drive is rather bizarre in the node realm. I'm not sure what the ramifications will be for not normalizing things, given that specific work was done around making that happen. for your use case, why not use the Windows equivalent of a symlink?

[Robinfr]

I understand it's rather bizarre, but this is unfortunately a requirement we cannot work around as our product is shipped with specific node modules that can be used from a different drive.

The code that is here seems very specific to normalizing for Windows drives, yet it does not seem to work properly since it does not work across drives, and most likely is not needed when it's on the same drive. The test for it is unfortunately not really doing much since it's simply testing against the same algorithm that's being used for normalizing (I understand it's not actually possible to test for this scenario using normal CI).

What was the specific issue that was trying to be solved by adding this piece of code?

[shellscape]

What was the specific issue that was trying to be solved by adding this piece of code?

"View git blame"

Since that was a straight migration with no updates, you'll then look here: https://github.com/rollup/rollup-plugin-alias/blame/0204677c0e1112cba7a5a0f60204ff1df81493b0/src/index.js#L8

(Which will eventually lead you here rollup/rollup-plugin-alias#3 and the associated PR rollup/rollup-plugin-alias#6)

If stuff was horribly, historically wrong, then I'm open to the change. But you're describing an incredibly niche edge case that calls for removal of code that was put there to solve larger problems. Without knowing more, it looks an awful like the architect in charge of that product doesn't fully understand the Node/NPM ecosystem and made some questionable product arch decisions. Food for thought: https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/

[shellscape]

Added some folks as reviewers to review the conversation thus far. If this receives no further comment for 30 days, we'll unfortunately have to decline the PR.

[danielgindi]

Viewing git blame, and issues history, I still don't see a justification for the original change that caused the currently discussed bug.
Furthermore, the fact that it's currently deviating from how the whole rollup ecosystem describes and formats ids- is worrying to me.

If anyone sees an actual issue that was resolved by the original change, then this issue should be clearly described and a test for it should be added.

Otherwise, I vote for merging this PR.

[shellscape]

Otherwise, I vote for merging this PR.

That's all the justification I think we need. Thanks for taking a look @danielgindi.