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

Incompatibility with global-style #151

Open
mvastola opened this issue Sep 15, 2018 · 6 comments
Open

Incompatibility with global-style #151

mvastola opened this issue Sep 15, 2018 · 6 comments

Comments

@mvastola
Copy link

So this just caused me major headache: if you have global-style active in your npmrc, browserify won't find dependencies of your dependencies.

It would be very helpful if browserify were able to locate these modules even if they are in subdirectories of node_modules. At the very least, a warning would be immensely helpful (perhaps at package install time) if global-style is set.

Another idea is to check package-lock.json. It seems to keep track of if global-style is being used, so checking it could either be used to generate a warning, or to resolve the actual location of a dependency.

To reproduce this issue, simply create a package with the following package.json:

{
  "name": "global-styles-issue",
  "version": "1.0.0",
  "private": true,
  "browserify": {
    "transform": [
      "babelify"
    ]
  },
  "dependencies": {
    "@babel/core": "^7.0.1",
    "babelify": "^10.0.0",
    "browserify": "^16.2.2"
  }
}

.. and then create a JS file with:

#!/usr/bin/env node
'use strict';
const fs = require('fs');
const path = require('path');
const browserify = require('browserify');

const testFile = path.resolve(__dirname, 'test.js');
let fd = fs.openSync(testFile, 'a');
fs.closeSync(fd);
browserify(testFile).bundle()

Finally, run npm i only after turning global-styles on.

@goto-bus-stop
Copy link
Member

Could you share the error you're getting? I can't reproduce this. Dependencies of dependencies are resolved from their own directory, ie. doing require('foo') inside node_modules/bar will first check node_modules/bar/node_modules/foo, and then walk up the directory tree. It should support global-style, which was the default before npm v3.

@mvastola
Copy link
Author

Ok. That was my fault. I oversimplified the issue away, though it was still causing an error on my end due to a file I failed to clean up.

I'm having real trouble drilling this down to a minimal working example though. (Or at least I've spent like 5 hours trying to do so already.) My suspicion though is there isn't too much that can be peeled away to still show this error (at least in terms of the complexity of the directory organization).

What I was trying to do was build the app Frida. One module in this app is called frida-gum. I've been able to reduce the issue to these steps thus far:

  • git clone https://github.com/frida/frida-gum
  • cd frida-gum/bindings/gumjs
  • npm i
  • ./node_modules/.bin/frida-compile ./runtime/entrypoint-duktape.js -o runtime -c

(This last command is effectively run during a build step, but these steps are simpler than compiling the whole thing.)

This all works fine is global-style is off, but is broken when it is on.

Note: There is arguably a bug in index.js of the frida-compile module. (I wanted to get to the root of this problem though before reporting it.) You should probably remove line 156, which consists of basedir: process.cwd(),.

Even after doing that though, the following error results:

events.js:167
      throw er; // Unhandled 'error' event
      ^

Error: Cannot find module 'uglifyify' from '/home/mvastola/src/frida-gum/bindings/gumjs'
    at /home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/resolve/lib/async.js:51:31
    at processDirs (/home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/resolve/lib/async.js:185:39)
    at ondir (/home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/resolve/lib/async.js:200:13)
    at load (/home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/resolve/lib/async.js:83:43)
    at onex (/home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/resolve/lib/async.js:108:17)
    at /home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/resolve/lib/async.js:12:69
    at FSReqWrap.oncomplete (fs.js:154:21)
Emitted 'error' event at:
    at /home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/browserify/index.js:337:34
    at /home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/resolve/lib/async.js:53:13
    at processDirs (/home/mvastola/src/frida-gum/bindings/gumjs/node_modules/frida-compile/node_modules/resolve/lib/async.js:185:39)
    [... lines matching original stack trace ...]
    at FSReqWrap.oncomplete (fs.js:154:21)

@goto-bus-stop
Copy link
Member

gumjs specifies the babelify transform in its package.json, but it's missing in the dependencies key. it only works accidentally with npm 3 and up because of flattening, but won't work with global-style or alternative clients like pnpm. transforms must be installed in the node_modules directory next to the package.json (or a node_modules in a parent directory), just like any other package.

@mvastola
Copy link
Author

  1. Interesting. Is this a browserify limitation, or part of the package.json spec? It seems a bit counterintuitive that this is required here because browserify is actually called from the frida-compile dependency, and frida-compile does have all the required dependencies. The root package doesn't even require browserify -- the line in package.json is just to store preferences, essentially, it seems.

  2. I don't think the entry in package.json is the extent of the issue. I just deleted that key and I'm still getting the same error. I should also note the package.json only ever had the babelify transform listed, and the error I'm stuck on is for uglifyify.

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Sep 17, 2018

It's how the resolution algorithm works—just like how in CJS files, require('babelify') is resolved from the directory it's called in, transforms in package.json are resolved from the package.json directory.

I'm not sure about uglifyify, but I don't see it anywhere in the code samples you posted. I would guess there is another similar problem inside a frida-compile related module.

e; Specifically i would guess there is a b.transform('uglifyify') call in a frida module somewhere. Tools should generally do b.transform(require('uglifyify')) instead to ensure that resolution works correctly, because browserify doesn't know which file b.transform() is called from. It defaults to the current working directory which is the correct behaviour for the browserify CLI (-t uglifyify becomes b.transform('uglifyify')) but which isn't good for most other situations. Unfortunately there is really no good default for any other situations though, AFAICT.

@mvastola
Copy link
Author

mvastola commented Sep 17, 2018

I'm not sure about uglifyify, but I don't see it anywhere in the code samples you posted. I would guess there is another similar problem inside a frida-compile related module.

Yes it's on line 216 of bindings/gumjs/node_modules/frida-compile/index.js

to ensure that resolution works correctly, because browserify doesn't know which file b.transform() is called from. It defaults to the current working directory which is the correct behaviour for the browserify CLI

I guess where I'm lost is that browserify is constructed in bindings/gumjs/node_modules/frida-compile/index.js. uglifyify is in bindings/gumjs/node_modules/frida-compile/node_modules. If my opts.basepath is defined (in bindings/gumjs/node_modules/frida-compile/index.js), shouldn't that path and all paths above it be searched for uglifyify as well as browserify? Theoretically, doing this shouldn't interfere with the CLI, where I don't think it's possible to set a basedir (and even if it is, it would seem reasonable to require all requires to be in the basedir or a parent directory).

I'm not 100% fluent in this though, so I could be missing something. Am I?

Thanks so much for all your help.

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