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

RP2.0, in browser - Error when importing request-promise as an ES6 module #91

Closed
bobbysciacchitano opened this issue Feb 9, 2016 · 16 comments

Comments

@bobbysciacchitano
Copy link

When I attempt to import request-promise into my web app I get the following error (using Browserify and Babel to transpile):

Cannot find module 'cls-bluebird' from '/Users/Bobby/Documents/Development/mediabase/node_modules/request-promise/lib'

My package.json (snipped) is as follows:

  "devDependencies": {
    "babel-preset-es2015": "^6.5.0",
    "babel-preset-react": "^6.5.0",
    "babelify": "^7.2.0",
    "browserify": "^13.0.0",
    "del": "^2.2.0",
    "gulp": "^3.9.1",
    "gulp-concat": "^2.6.0",
    "gulp-connect": "^2.3.1",
    "gulp-less": "^3.0.5",
    "history": "^1.17.0",
    "normalize-css": "^2.3.1",
    "react": "^0.14.7",
    "react-dom": "^0.14.7",
    "react-router": "^1.0.3",
    "vinyl-buffer": "^1.0.0",
    "vinyl-source-stream": "^1.1.0"
  },
  "dependencies": {
    "express": "^4.13.4",
    "request-promise": "2.0.0"
  }

Commenting out the request.bindCLS line in the library seems to fix the issue with no adverse affect on request-promise. If I use a standard require() to load the module I get the same issue. Do I need to include cls-bluebird as a dependency ? I thought this was removed ...

@analog-nico
Copy link
Member

Hi @bobbysciacchitano this feature was not removed but if you don't call .bindCLS(...) no additional code is executed and you don't need this extra dependency. Basically, it is save to comment out the code. However, you don't want to do this of course. What you need to do instead is to tell Browserify to ignore the dependency.

I am using such a mechanism in Webpack and it looks like this:

{
  plugins: [
    new webpack.IgnorePlugin(/^\.\/locale$/, /moment$/)
  ]
}

I hope something similar is available for Browserify as well. Otherwise you would need to wait for request-promise@3 where .bindCLS(...) will be dropped. But the next version will not come soon.

If you find a solution for Browserify please post it here. Thanks!

@schweller
Copy link

@analog-nico Isn't a patch possible just to fix this? It's been a real pain have to deal with that and the webpack solution seems too hacky.

@analog-nico
Copy link
Member

Sorry, @schweller it is not possible unless we drop the CLS support altogether. You have to either wait for the next version or declare cls-bluebird as an "external" dependency in the Browserify config.

@schweller
Copy link

Hey @analog-nico, sorry to disturb once again about this matter, but I want to propose a patch solution before the next major release:

  • We export an experimental.js file that will go inside ./lib/;
  • The code inside the experimental.js could be something like that:
var request = require('./rp')

/* istanbul ignore next */ // Function covered but not seen by Instanbul.
request.bindCLS = function RP$bindCLS(ns) {
  require('cls-bluebird')(ns);
};

module.exports = request;
  • And now the experimental feature would be optional and not mandatory. If you want, just include normally:
//ES6 modules
import rp from 'request-promise/lib/experimental'

//Using require
var rp = require('request-promise/lib/experimental')

//Without experimental feature
import rp from 'request-promise'

I already tested this solution locally and it works fine, but I'm forking the project so I can make a PR later today or tomorrow.
IMO, It seems better than having everybody download an extra dependency or declare it as an external in Browserify or Webpack.

Let me know if you have any concerns.

@analog-nico
Copy link
Member

Hi @schweller I prefer not to bump the main version yet again to not irritate the users regarding breaking changes.

I googled a bit more and found out that you could use the browser field in your package.json:

{
  "browser": {
    "cls-bluebird": false
  }
}

But here is my offer: I will cut out enough time to work on the next version soon and you could help with officially supporting Browserify. I could use a helping hand!

@bobbysciacchitano
Copy link
Author

Hi thanks for the updates:

For the benefit of others using Browserify the ignoreMissing / ignore-missing flag did the trick for me as a temporary workaround.

Alternatively:

bundler.ignore('cls-bluebird');

@analog-nico
Copy link
Member

Thanks for the info @bobbysciacchitano ! I'll keep this issue open until even bundler.ignore('cls-bluebird'); is not needed anymore.

@lourd
Copy link

lourd commented Mar 15, 2016

Just ran into this issue as well using Webpack, pasting my error and solutions for those who follow in my google steps:

ERROR in ./~/request-promise/lib/rp.js
Module not found: Error: Cannot resolve module 'cls-bluebird'

Fix is adding new webpack.IgnorePlugin(/cls-bluebird/, /request-promise/) to your plugins array.

@fdocr
Copy link

fdocr commented Apr 7, 2016

Hi all, I'm using Browserify with npm scripts so bundler.ignore('cls-bluebird'); was not an option for me. The browser ignore field inside my package.json didn't work in my case, not sure why since I'm not an experienced Browserify user and currently working in a quirky project.

My solution was to use -i cls-bluebird flag to emulate bundler.ignore('cls-bluebird') instead of ignoreMissing / -im

Sharing this suggestion as a follow up since ignoring all missing modules might result in modules not loaded in case of a silly error (typo or some other weird thing) and would end up ignored AFAIK.

This extra ignored errors aren't a catastrophe for a temporary solution, but it might be safer (and easier to debug) this way. It won't break other public modules apart from this short list. Thanks!

@analog-nico
Copy link
Member

Thanks for the info @fdoxyz !

@fotinakis
Copy link

For those using ember-browserify, you can add this to your config/environment.js to fix this:

    browserify: {
      ignores: ['cls-bluebird'],
    },

-- Very unfortunately you have to manually include this in each host app though, so it would be really nice if request-promise could make browserifying easier without the need to do these things.

@analog-nico
Copy link
Member

Thanks @fotinakis !

I will remove cls-bluebird with the next release in a few weeks. (And there will be an alternative way for those who actually need cls-bluebird.)

@brandonbowersox
Copy link

If you're using grunt-browserify, here is how to add the ignore into your Gruntfile.js:

browserify: {
    options: {
        ignore: ['cls-bluebird']
    }
}

See the browserify documentation about ignore and exclude.

@nathanph
Copy link

nathanph commented Jul 8, 2016

@analog-nico

"The browser field only applies to the current package. Any mappings you put will not propagate down to its dependencies or up to its dependents. This isolation is designed to protect modules from each other so that when you require a module you won't need to worry about any system-wide effects it might have. Likewise, you shouldn't need to wory about how your local configuration might adversely affect modules far away deep into your dependency graph."

https://github.com/substack/browserify-handbook#browser-field

Thus to ignore cls-bluebird we need to modify the package.json inside <app>/node_modules/request-promise/.

Same code:

{
  "browser": {
    "cls-bluebird": false
  }
}

NOTE: This isn't terribly graceful. If you reinstall packages after this it will override your changes to the package.json. It's also not a portable solution. Let me know if you can think of another solution.

@analog-nico
Copy link
Member

@nathanph Thanks for the info!

Well, having the cls-bluebird dependency obviously turned out to be a pita to begin with. I am currently working on the next version of Request-Promise. In this version cls-bluebird will be dropped altogether. So imho it is best to wait for this version rather than trying to find a better fix for the existing mess... 😉

@analog-nico
Copy link
Member

Hey buddies, I just released request-promise@4.0.0 that dropped the whole cls-bluebird part. You should have no issues with webpack and browserify anymore.

Thanks everyone for providing workarounds until now.

adhambadr added a commit to adhambadr/node-forecastio that referenced this issue Feb 17, 2017
epatters added a commit to epatters/datascienceontology-backend that referenced this issue Oct 18, 2018
This can be removed when the new version of openwhisk is released,
which switches from request-promise to needle.

References:

request/request-promise#91
apache/openwhisk-client-js#78
epatters added a commit to epatters/datascienceontology-backend that referenced this issue Dec 1, 2019
This can be removed when the new version of openwhisk is released,
which switches from request-promise to needle.

References:

request/request-promise#91
apache/openwhisk-client-js#78
epatters added a commit to epatters/datascienceontology-backend that referenced this issue Dec 1, 2019
This can be removed when the new version of openwhisk is released,
which switches from request-promise to needle.

References:

request/request-promise#91
apache/openwhisk-client-js#78
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants