Add option to not follow symlinks #1819

kellyselden · 2017-12-27T18:47:55Z

I believe symlinks used to be preserved, but now are followed. This is hurting me because in broccolijs land, everything is a symlink. This makes using the node-resolve plugin a nightmare.

Can we discuss an opt-in option for preserving symlinks? This PR is just a sample of what would change, not a final product. I would like the option to by system-wide public API, because several rollup plugins also follow rollup's example and follow symlinks. It would be nice if they could inherit the public option.

kellyselden · 2018-01-05T18:47:44Z

I added the opt-in option and added a test. It is also based on #1820 so all plugins can also read this option.

kzc · 2018-01-07T21:51:05Z

Is there a CLI flag for this setting without needing a config file? I can't get it to work.

kellyselden · 2018-01-07T22:05:59Z

@kzc I forgot about the CLI when I wrote this. It is just a config.

kzc · 2018-01-07T22:30:08Z

In light of this description it's surprising that Rollup has the "realpath" node module resolution symlink behavior by default. It seems to be a bug. This PR should not be needed if Rollup behaved as described.

The node realpathSync stuff should only be in rollup-plugin-node-resolve, not in core rollup.

guybedford · 2018-01-08T12:52:20Z

Agreed this sounds like a bug to me. The default behaviour should ideally match Node resolution which would not be to use realpath, and rather have that be through a flag or plugin.

kzc · 2018-01-10T22:32:07Z

@Rich-Harris Was the use of node's realpath module resolution in Rollup core intentional?

rollup/src/utils/defaults.ts

Line 13 in ddb698c

if (stats.isSymbolicLink()) return findFile(realpathSync(file));

Rich-Harris · 2018-01-10T22:56:41Z

I can't recall to be honest, perhaps I was dealing with some tricky npm link scenario and that seemed to fix it... if the consensus is that we shouldn't use it then I'm on board 👍

guybedford · 2018-01-11T12:45:25Z

Perhaps we can consider then making preserveSymlinks the default behaviour to match Node.

This option could then either be preserveSymlinks: false to opt-out, or the inverse - realpath: true.

kellyselden · 2018-01-11T16:55:48Z

@guybedford Would that be an acceptable breaking change?

kzc · 2018-01-11T17:24:09Z

Perhaps we can consider then making preserveSymlinks the default behaviour to match Node.

I find the phrase "to match node" to be a bit ambiguous. At the risk of restating what you wrote, I suggest by default Rollup should behave like node --preserve-symlinks. The use of realpath in Rollup should be an explicit opt in - or ideally be handled by rollup-plugin-node-resolve exclusively.

(Off topic: I wish that node flag had been named --ignore-symlinks as its intent was to treat symlinked files and directories as regular files and directories rather than invoking the confusion of "preserving" legacy node realpath module resolution.)

guybedford · 2018-01-11T17:38:50Z

Thanks @kzc for the clarifications and sorry for the confusion here, I got this the wrong way around then.

In that case, this PR exactly as it stands sounds like exactly the right approach to me, as the Node behaviour should be the default followed behaviour.

kzc · 2018-01-11T17:44:18Z

In that case, this PR exactly as it stands sounds like exactly the right approach to me, as the Node behaviour should be the default followed behaviour.

Respectfully, I beg to differ. I think by default Rollup should not use "realpath" as this PR does. "realpath" should be an explicit opt-in.

Regardless of the default chosen, a Rollup CLI flag should be added to enable or disable "realpath" resolution.

guybedford · 2018-01-11T17:51:08Z

Ugh, I will make sure to think before typing here! Comment removed for inaccuracies until I can actually take the time to put together a coherent thought :P

kzc · 2018-01-11T17:55:54Z

@guybedford No worries. The entire topic of "preserve symlinks" and node "realpath" module resolution is very confusing in general.

guybedford · 2018-01-12T11:31:08Z

So my the last deleted comment was on track with the arguments, I just got the wrong interpretation of symlink handling in NodeJS.

Basically, NodeJS follows symlinks to their real paths because when you link packages in a development workflow, you need to get the guarantee that the linked package dependencies will be used, so that multi-version between linked projects works.

Rollup should follow NodeJS here so that this same invariant is maintained when running Rollup in linking workflows this way.

This PR implements preserveSymlinks: true as an option to not use realpath, which then exactly matches NodeJS.

So my recommendation is to merge this as-is.

Apologies for confusions :)

kzc · 2018-01-12T17:30:23Z

This PR implements preserveSymlinks: true as an option to not use realpath, which then exactly matches NodeJS.

@guybedford Just to confirm, did you mean the following?

"This PR implements preserveSymlinks: true as an option to not use realpath, which then exactly matches the behavior of node --preserve-symlinks"

Node in its default mode uses "realpath" module resolution (not to be confused with absolute path). If node --preserve-symlinks is run then node will not use "realpath" module resolution and treat symlinked files as if they were regular files and directories and resolve to absolute paths. This PR in its present state would have Rollup continue to use "realpath" module resolution by default.

Although I happen to disagree with the default chosen by this PR to retain "realpath" module resolution by default, I would still recommend this PR implement a command line flag to override this "realpath" behavior globally rather than having it solely settable via rollup config.

kellyselden · 2018-01-12T17:55:50Z

Although I happen to disagree with the default chosen by this PR to retain "realpath" module resolution by default, I would still recommend this PR implement a command line flag to override this "realpath" behavior globally rather than having it solely settable via rollup config.

@kzc I'll do that once consensus is made on whether it is opt-in or opt-out.

kzc · 2018-01-13T20:23:08Z

Whatever @guybedford decides for default behavior is fine with me. No doubt some Rollup projects are relying on the default "realpath" module resolution behavior.

guybedford

A couple of comments in the code.

I'm tending towards saying we should make preserveSymlinks opt-in exactly as we have here to continue match both NodeJS and other bundlers.

But I'd still be open to discussing use cases further here as to what is best for Rollup. The defaults seem to be set to building "libraries" which may benefit from symlinks. But ideally I'd like to see the defaults all matching NodeJS for easier first-use (even including NodeJS resolution and CommonJS plugin). Although I can hear @Rich-Harris rolling his eyes at this already...

guybedford · 2018-01-15T12:23:58Z

src/utils/first.ts

 		return candidates.reduce((promise, candidate) => {
 			return promise.then(
 				result =>
-					result != null ? result : Promise.resolve(candidate(...args))
+					result != null ? result : Promise.resolve(candidate.apply(this, args))


Are these changes intended?

It comes from #1820 (which should be merged first IMO). It is so plugins that also do realpath (multi-entry, node-resolve) can read the option via this.preserveSymlinks and act accordingly, instead of each having their own duplicate options.

It's also needed for https://github.com/rollup/rollup/pull/1819/files#diff-9b4fdd2a9bd0304295b89fa6270add84R54 to have this context.

guybedford · 2018-01-15T12:24:47Z

src/utils/mergeOptions.ts

@@ -74,6 +74,7 @@ export default function mergeOptions ({
 		cache: getInputOption('cache'),
 		preferConst: getInputOption('preferConst'),
 		experimentalDynamicImport: getInputOption('experimentalDynamicImport'),
+		preserveSymlinks: config.preserveSymlinks,


getInputOption here?

(as far as I can tell this would enable --preserveSymlinks in CLI but I may be reading this wrong?)

I wasn't able to get the CLI option working. Is there some special syntax required?

Did you build with getInputOption('preserveSymlinks')?

No. I thought you were referring to the code as it exists.

Changed to getInputOption.

lukastaegert

I would be willing to merge this PR if it were not for the use of this to access the bundle options. Unfortunately this works against any efforts to further unify the plugin API with regard to the this context.

Currently, the transform hook uses the context to enable plugins to throw warnings via this.warn. IMO, this should be extended to all hooks to resolve #1501. Any PR that works towards this goal would be heartily welcomed.

For the problem at hand, however, replacing the context is totally unnecessary. Instead, the signature of resolveId in defaults.ts could be changed to

export function resolveId ({ preserveSymlinks }) { // possibly other options as well
  return (importee: string, importer: string) => {
    ...
    // previous implementation that now has direct access to options
  }
}

as when the function is used in Graph.ts, options are already available and can be used for currying. As for external plugins, they could always use the options hook. Additionally, we could consider adding the inputOptions to the context as well even though, as I pointed out above, this is not necessary at all for the current problem.

So to sum it up:

Do not expose the bundle via context to gain access to options
Instead, either use currying (less invasive for now and certainly easier to implement) or implement a more consistent plugin API as outlined above

kellyselden · 2018-01-21T19:36:21Z

I will take out the dependency on the first context.

kellyselden · 2018-01-28T04:19:04Z

rebased, and used curried options to remove dependency on this. Let me know what you think @lukastaegert

lukastaegert

👍

lukastaegert · 2018-01-29T07:50:03Z

@kellyselden Just checked your test and it does not turn red if I change the "preserveSymlinks" option. Which is probably because git did not restore any symlinks for me. As the answers here sound like this should actually work, could you please check on your side that the symlink is actually correctly checked in?

kellyselden · 2018-01-29T21:25:45Z

@lukastaegert Fixed the test. When I rebased in Windows, it must have destroyed the symlink...

lukastaegert · 2018-02-01T06:47:08Z

It's working now, thanks 👍

kellyselden force-pushed the symlinks branch from 727dd18 to d258687 Compare January 5, 2018 18:45

kellyselden mentioned this pull request Jan 5, 2018

RFC: preserve context in first #1820

Closed

kellyselden force-pushed the symlinks branch 5 times, most recently from fac76c2 to a66dffc Compare January 6, 2018 03:33

kzc mentioned this pull request Jan 7, 2018

Rollup treats linked modules as external dependencies. #1822

Closed

guybedford approved these changes Jan 15, 2018

View reviewed changes

kzc mentioned this pull request Jan 15, 2018

docs: clarify rollup-plugin-node-resolve #1559

Closed

kellyselden force-pushed the symlinks branch from a66dffc to 853fb77 Compare January 16, 2018 03:19

kzc mentioned this pull request Jan 19, 2018

[Question] why not node resolve as default ? #1895

Closed

lukastaegert requested changes Jan 21, 2018

View reviewed changes

kellyselden force-pushed the symlinks branch 2 times, most recently from 37c55d6 to 0a5796b Compare January 28, 2018 04:17

lukastaegert approved these changes Jan 29, 2018

View reviewed changes

lukastaegert added this to the 0.55.2 milestone Jan 29, 2018

lukastaegert changed the title ~~RFC: add option to not follow symlinks~~ Add option to not follow symlinks Jan 29, 2018

kellyselden force-pushed the symlinks branch from 0a5796b to d0b9a30 Compare January 29, 2018 21:25

add option to not follow symlinks

d0b9a30

lukastaegert merged commit d0b9a30 into rollup:master Feb 1, 2018

kellyselden deleted the symlinks branch February 1, 2018 08:05

Add option to not follow symlinks #1819

Add option to not follow symlinks #1819

Conversation

kellyselden commented Dec 27, 2017

kellyselden commented Jan 5, 2018

kzc commented Jan 7, 2018

kellyselden commented Jan 7, 2018

kzc commented Jan 7, 2018 • edited

guybedford commented Jan 8, 2018

kzc commented Jan 10, 2018

Rich-Harris commented Jan 10, 2018

guybedford commented Jan 11, 2018

kellyselden commented Jan 11, 2018

kzc commented Jan 11, 2018 • edited

guybedford commented Jan 11, 2018

kzc commented Jan 11, 2018

guybedford commented Jan 11, 2018

kzc commented Jan 11, 2018

guybedford commented Jan 12, 2018

kzc commented Jan 12, 2018

kellyselden commented Jan 12, 2018

kzc commented Jan 13, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford Jan 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert left a comment

Choose a reason for hiding this comment

kellyselden commented Jan 21, 2018

kellyselden commented Jan 28, 2018

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert commented Jan 29, 2018

kellyselden commented Jan 29, 2018

lukastaegert commented Feb 1, 2018

kzc commented Jan 7, 2018 •

edited

kzc commented Jan 11, 2018 •

edited

guybedford Jan 15, 2018 •

edited