rnpm / rnpm-plugin-link

Allows linking dependencies to your React Native project
MIT License
31 stars 19 forks source link

Chore/dynamic require error handling #56

Closed Kureev closed 8 years ago

Kureev commented 8 years ago

Side-track fix: correct repo details in the package.json. Feel free to ask me to exclude it form this PR (out of scope).

grabbou commented 8 years ago

So the error is coming from the fact there's no such folder with given prefix or that file throws under some circumstances?

Kureev commented 8 years ago

Nope, the error comes from the developer issue. I mean if we have a typo or variable doesn't exist.

Kureev commented 8 years ago

Can we merge it in?

grabbou commented 8 years ago

It's still unclear to me why we have a try catch there from plain require. On master we now run these functions inside a promise with a global catch at the end, so shouldn't that just work? https://github.com/rnpm/rnpm-plugin-link/blob/master/src/link.js#L125-L131

Is there any issue or a reproduction I can try to see what's the problem that fixes?

Kureev commented 8 years ago

If you compose a require path in the application lifecycle, node will swallow any errors that this require will throw.

Example:

a.js:

some shit there

b.js:

function dynamicRequire(filename) {
  return require('./' + filename);
}

dynamicRequire('a'); // no errors
Kureev commented 8 years ago

Is there any issue or a reproduction I can try to see what's the problem that fixes?

When something in dynamic require fails (like ./${prefix}/makeMainActivityPatch), then it becomes really hard to debug.

grabbou commented 8 years ago

Ok, thanks, will throw an error and see what happens 🎱

Kureev commented 8 years ago

Did you managed to reproduce it?

Kureev commented 8 years ago

I think I can probably come up with a better structural solution than this one. We can close it for now.

grabbou commented 8 years ago

I put throw new Error('Test') in one of the patches then run link and I got proper stack trace. So I can't reproduce it.

/Users/grabbou/Repositories/rnpm/rnpm-plugin-link/src/android/patches/0.20/makePackagePatch.js:5
  throw new Error('Test');
  ^

Error: Test
    at makePackagePatch (/Users/grabbou/Repositories/rnpm/rnpm-plugin-link/src/android/patches/0.20/makePackagePatch.js:5:9)
    at unregisterNativeAndroidModule (/Users/grabbou/Repositories/rnpm/rnpm-plugin-link/src/android/unregisterNativeModule.js:40:5)
    at Object.unlink [as func] (/Users/grabbou/Repositories/rnpm/rnpm-plugin-link/src/unlink.js:46:30)
    at Command.runAction (/Users/grabbou/Repositories/rnpm/rnpm/bin/cli:21:15)