Skip to content
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

td.replace does not follow node_modules symlinks since v3.3.2 #324

Closed
arelra opened this issue Jan 17, 2018 · 8 comments
Closed

td.replace does not follow node_modules symlinks since v3.3.2 #324

arelra opened this issue Jan 17, 2018 · 8 comments

Comments

@arelra
Copy link

arelra commented Jan 17, 2018

Hi,

Since v3.3.2 td.replace() does not appear to follow symlinks in local node_modules.

This is a particular problem when using mono repo tooling such as Lerna

For example:

repo
  -- packages
     -- packageA
     -- packageB (dependency on packageA)
           /node_modules/packageA (symlink)
           test.js has a test with td.replace('packageA')

Lerna will create a symlink for packageB to packageA ie packageB/node_modules/packageA

td.replace does not appear to recognise the path and throws the following exception:

  Error {
    code: 'MODULE_NOT_FOUND',
    message: 'Cannot find module \packageA\'',
  }

I'm assuming because resolve behaviour was changed recently?

Thanks

@searls
Copy link
Member

searls commented Jan 17, 2018

Yeah, this was caused by 9f51110 and wasn't a foreseen consequence of switching to the built-in require module's require.resolve() function.

In the process of attempting to eliminate the same browserify/resolve dependency from testdouble's dependency quibble, we discovered that there is no preserveSymlinks option for require.resolve, which we're looking at adding in nodejs/node#18009 . However, fixing this in Node 9+ doesn't do much given our support for Node 4+, so I think we should revert the change and continue to rely on browserify/resolve

@arelra
Copy link
Author

arelra commented Jan 17, 2018

Thank you @searls for your prompt response and all your work on testdouble 👍

@searls
Copy link
Member

searls commented Jan 17, 2018

I need some help replicating this issue. If you'll look at this branch & example project, and attempt running npm it, the test file passes despite the replaced module being linked.

https://github.com/testdouble/testdouble.js/tree/324-fix-preserve-symlinks/examples/324-symlink-module-regression

Can you take a look and adjust it so that it fails?

@arelra
Copy link
Author

arelra commented Jan 17, 2018

Running that test with no modifications actually gives me an exception:

module.js:471
    throw err;
    ^

Error: Cannot find module 'symlinked-thing'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.resolve (internal/module.js:27:19)
    at requireAt (/projects/personal/testdouble.js/lib/replace/module.js:48:28)
    at exports.default (/projects/personal/testdouble.js/lib/replace/module.js:11:19)
    at exports.default [as replace] (/projects/personal/testdouble.js/lib/replace/index.js:9:29)
    at Object.<anonymous> (/projects/personal/testdouble.js/examples/324-symlink-module-regression/test.js:4:25)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)

Perhaps its environment/version specific? I'm on:

npm ERR! node v6.9.5
npm ERR! npm  v3.10.10

@arelra
Copy link
Author

arelra commented Jan 17, 2018

@searls

If it helps I've created a simple mono-repo with symlinks that reproduces the problem:

https://github.com/arelra/testdouble-issue-324-symlink-module-regression

npm it

Which produces the error:

module.js:471
    throw err;
    ^

Error: Cannot find module 'package-a'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.resolve (internal/module.js:27:19)
    at requireAt (/projects/personal/testdouble-issue-324-symlink-module-regression/node_modules/testdouble/lib/replace/module.js:48:28)
    at exports.default (/projects/personal/testdouble-issue-324-symlink-module-regression/node_modules/testdouble/lib/replace/module.js:11:19)
    at exports.default [as replace] (/projects/personal/testdouble-issue-324-symlink-module-regression/node_modules/testdouble/lib/replace/index.js:9:29)
    at Object.<anonymous> (/projects/personal/testdouble-issue-324-symlink-module-regression/packages/package-b/test.js:4:25)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
npm ERR! Test failed.  See above for more details.

node v6.9.5
npm v3.10.10

@searls
Copy link
Member

searls commented Jan 18, 2018

Hey @arelra, I'm a bit frustrated because I sunk a couple hours into this last night (I'd never used lerna so there were some false starts, and I couldn't understand why this wouldn't work when other symlinked module paths were working fine) based on your statements that v3.3.2 introduced this.

I just manually tested your repo against 15 releases (from v1.9.1 forward), and this usage doesn't work with any of them. Can you please provide an example in which your lerna case is actually working? If we can't, then let's call this an enhancement and throw it on the backlog.

searls added a commit that referenced this issue Jan 18, 2018
This reverts commit 9f51110.

We are reverting because require.resolve() does not currently
preserve symlinks unless the entire process is configured to,
and that setting cannot be set on the function or once the process
is spun up. So, for now we are going to keep using the resolve
module.

Reference:
#324
@arelra
Copy link
Author

arelra commented Jan 18, 2018

Hi @searls Thank you for taking the time to look at the problem.

I've had a closer look through our codebase and you're right we have previously only td.replaced dependencies that were hoisted by Lerna but not symlinked so this has probably always been an issue.

I first saw the exception when trying to replace a symlinked module and v3.3.2 was pulled in which changed the way modules were resolved internally which mislead me.

Apologies for not seeing this sooner.

Yes please add resolution of symlinks in this scenario as an enhancement.

Thanks again.

@searls
Copy link
Member

searls commented Jan 18, 2018

Closing in favor of #326. For what it's worth, take care to notice that the root issue likely doesn't have anything to do with symlinks per se, but rather cwd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants