Add experimental support for symlinked modules #231
Conversation
When linking external dependencies using `npm_link`, rollups commonjs plugin does not process files in the symlinked directory correctly. To support this, rollup need the `preserveSymlinks` setting to be true. Since we cannot assess at the moment whether this setting will break some other build, we put it behind an experimental flag. So you need to set the environment varialbe `FAUCET_EXPERIMENTAL_SYMLINKS` to `"true"` to enable preserving symlinks.
There was a problem hiding this comment.
This is a good PR, thank you for that!
Just a few, mostly minor observations.
Seeing it like that makes me wonder whether the unofficial hack via an environment variable is really necessary. I know I was the one to propose that in order to avoid expanding the official API's surface area, but now I'm wondering whether we might just turn it into a regular configuration option. That of course means finding a suitable name, consistent with established patterns, and adding documentation...
I'd also be interested in @moonglum's opinion, if they have the time and inclination.
| @@ -88,8 +88,13 @@ function generateConfig({ externals, format, exports, // eslint-disable-next-lin | |||
|
|
|||
| plugins = plugins.concat([ | |||
| nodeResolve({ extensions }), | |||
| commonjs({ include: "node_modules/**" }) | |||
| commonjs({ include: /node_modules/ }) | |||
There was a problem hiding this comment.
This seems like a potentially significant change? I vaguely remember pondering this expression extensively (years ago though), so I'm hesitant to change it without asking: What's the significance in this particular context?
| @@ -3,6 +3,7 @@ | |||
|
|
|||
| let { MockAssetManager, makeBundle, FIXTURES_DIR } = require("./util"); | |||
| let faucetJS = require("../../lib").plugin; | |||
| const fs = require("fs"); | |||
There was a problem hiding this comment.
| const fs = require("fs"); | |
| let fs = require("fs"); |
for consistency 🙂
| @@ -0,0 +1,3 @@ | |||
| exports.dummy = { | |||
| some: "dummy value" | |||
| }; | |||
There was a problem hiding this comment.
FYI: We use underscores instead of hyphens in file and directory names.
| process.env.FAUCET_EXPERIMENTAL_SYMLINKS = initialFlagValue; | ||
| }); | ||
|
|
||
| it("should preserve symlinks when env varialbe is set", () => { |
There was a problem hiding this comment.
| it("should preserve symlinks when env varialbe is set", () => { | |
| it("should preserve symlinks when env variable is set", () => { |
typo 🙂
|
Great work, thanks @micha149 🙇
I would also say: Let's not introduce an environment variable because this would be (afaik) the first one we introduce. Let's stick with configuration options in the configuration file 👍 Apart from that and the nitpicks, this is ready to go from my point of view. |
When linking external dependencies using
npm_link, rollups commonjs plugin does not process files in the symlinked directory correctly. To support this, rollup need thepreserveSymlinkssetting to be true.Since we cannot assess at the moment whether this setting will break some other build, we put it behind an experimental flag. So you need to set the environment varialbe
FAUCET_EXPERIMENTAL_SYMLINKSto"true"to enable preserving symlinks.