Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Cannot import request-promise #823

Closed
dgreif opened this issue Sep 25, 2017 · 23 comments
Closed

Cannot import request-promise #823

dgreif opened this issue Sep 25, 2017 · 23 comments

Comments

@dgreif
Copy link
Contributor

dgreif commented Sep 25, 2017

request-promise uses request-promise-core, which does some interesting import paths internally. One of the imports is just a relative require('./errors.js'), which is breaking when bundled with fuse-box. Still an issue after #797. Here is an example using 2.3.1-beta.15: https://github.com/dgreif/request-promise-fuse

@nchanged
Copy link
Contributor

Just a side note why don’t you isolate dependencies for server? Are u deploying it to amazon lambda or something?

@dgreif
Copy link
Contributor Author

dgreif commented Sep 25, 2017

We are deploying with Docker, so we can definitely install dependencies. Would just be nice to deploy a bundle and not have to install dependencies. For now I'll bundle it without deps.

@calebboyd
Copy link
Contributor

I have this use-case also when deploying apps with nexe (node)

@nchanged
Copy link
Contributor

nchanged commented Sep 25, 2017

I don't think it's a good idea tho

  1. That defeats the purpose of using Docker and having environment specific binaries and builds.
  2. Many of the server libraries rely on the files system e.g __dirname and require other modules dynamically hence won't work in a bundle.

But I will take a look anyway

P.S it's not in request-promise it's some in place dark ;-)

@calebboyd
Copy link
Contributor

calebboyd commented Sep 25, 2017

Thanks for taking a look :)

To give a little more context:

  1. Bundling deps when you have docker is redundant yes. But Servers are not the only place that use the node runtime. Its everywhere, used to run all types of applications -- not just servers with docker ;) It could be any old application, on any machine. It is the developers responsibility to make sure they will be bundling the right code for a particular platform. Fusebox should still bundle whats there.
  2. Yeah, a lot of hacky ones do, people should stop using them ;-)

@nchanged
Copy link
Contributor

nchanged commented Oct 2, 2017

@dgreif @calebboyd
the latest beta works with this config:

QuantumPlugin({
            target : "server",
            uglify: false,
            bakeApiIntoBundle : "app",
            treeshake: true
        }),
import * as requestPromise from 'request-promise'

async function test () {
    const result = await requestPromise.get('http://www.google.com')
    console.log(result)
}

test()

I think you can package it in Quantum and deploy elsewhere.

@calebboyd
Copy link
Contributor

Hmm... Any idea why it doesn't work without quantum?

@nchanged
Copy link
Contributor

nchanged commented Oct 2, 2017

yes I've got an idea, Quantum removes conditions with document and window and for some reason, that library fails to detect if it's running on server - of course, module.exports is very much defined.

Long story short, it executes window

@calebboyd
Copy link
Contributor

I don't follow. The library has no references to any dom apis.

@nchanged
Copy link
Contributor

nchanged commented Oct 2, 2017

Sorry man, I had it with hmr() lol. Now I am getting that error with Error: Cannot find module './errors.js' I have zero clues why..

@calebboyd
Copy link
Contributor

Aye, Thats the original error.. Weird right?

@nchanged
Copy link
Contributor

nchanged commented Oct 2, 2017

I might have an idea, MIGHT be a circular dependency with this line

var core = require('../')

wherease there is no index.js and it takes the entry point. I am just debugging and seeing some odd stuff, as if there is not error.js but it is there... without evaluated context, as if there a circular reference somewhere, which we are not handling properly

@nchanged
Copy link
Contributor

nchanged commented Oct 2, 2017

Oh.. basePath is so wrong... ..js there is definitely something wrong with the loader......

@calebboyd
Copy link
Contributor

calebboyd commented Oct 2, 2017

I may have seen this before... Does it work if you change ../ to ..

Edit: no.. nvm

@nchanged
Copy link
Contributor

nchanged commented Oct 2, 2017

I remember somebody modified the loader, was it you? ;-) but it definitely fails to resolve that require("../") whereas there is no index.js
The final path it's trying is ..js - I guess out of desperation. And Quantum solves it nicely, hm....

Changing it to var core = require('.."') made if fail:

TypeError: core is not a function

It's really weird cuz there isn't any candidate it could reference to correctly.

https://github.com/request/promise-core/blob/master/configure/request2.js#L3

That one leads it to package.json Say we handle it properly (we can fix it) BUT. that one goes to lib/plumbin.js and i tried it too.. without any luck.

Any ideas?

@calebboyd
Copy link
Contributor

Yeah, I had this base case covered but after discussion we decided to narrow it. I think its safe to probably bring back...

We can change

if (!file && filePath === ".") {
file = pkg.f[pkg.s && pkg.s.entry];
}

To

        if (!file && isRelative.test(filePath)) {
            file = pkg.f[pkg.s && pkg.s.entry];
        }

Where isRelative is the same regex from PathMaster

const isRelative = /^[\.\/\\]+$/

@calebboyd
Copy link
Contributor

calebboyd commented Oct 2, 2017

Am I looking in the right place? It sounds like filePath is ".."

@nchanged
Copy link
Contributor

nchanged commented Oct 2, 2017

line ~ 5791ish

@nchanged
Copy link
Contributor

nchanged commented Oct 2, 2017

@calebboyd that didn't help ((

@calebboyd
Copy link
Contributor

calebboyd commented Oct 2, 2017

I wonder if it has to do with requiring a partial node_module from within a node_module, such that when looking up ./errors it retains the (incorrect) context of the original node_module

@nchanged
Copy link
Contributor

nchanged commented Oct 3, 2017

This is where it failes
console.log(require('request-promise-core/configure/request2'))

@nchanged
Copy link
Contributor

nchanged commented Oct 3, 2017

Ok, that was easy.. I am not sure how that was working before

if (!file && filePath === ".") {
             validPath = pkg.s && pkg.s.entry || "index.js";
             file = pkg.f[validPath];
          }

validPath was never set, therefore couldn't get resolved

@dgreif @calebboyd fuse-box@2.3.1-beta.24

@nchanged
Copy link
Contributor

nchanged commented Oct 3, 2017

🎆 ✌️

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

No branches or pull requests

4 participants