-
Notifications
You must be signed in to change notification settings - Fork 86
fix: NW.js module is not defined fix. #131
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
Conversation
Now the factory assignment for localforage is applied to `global.modules.exports` instead of `modules.exports` if `module` is undefined. `module` is undefined in an NW.js environment (for whatever reason). So now we check for module and we uses global if `module` is undefined. Otherwise we proceed as before. Thank @ashleyhindle for fix. Fixes scotttrinh#77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the typo, LGTM. Do you mind just require
ing from the node
repl to make sure it imports cleanly and letting me know it worked?
dist/angular-localForage.js
Outdated
if(typeof module === 'undefined') { | ||
global.module.exports = factory(angular, require('localforage')); // NW.js | ||
} else { | ||
modules.exports = factory(angular, require('localforage')); // Node/Browserify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here. Should be module.exports
not modules.exports
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the typo, but I am not sure what you want me to require on the REPL to check if it imports.
If it means anything I manually added my updated code to my working project using NW.js and the fix works in NW.js and in-browser.
Just want to make sure I am doing the right thing here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the typo!
I just mean do something like this on the command line (assuming macOS or linux):
$ cd LOCATION_OF_YOUR_FIXED_REPO
$ node
> var localForage = require('.')
Assuming it doesn't throw an error after that require
call, we should be good. If you're still confused, I can do it tonight when I get home from work. To be honest, it might error out since node doesn't have a LocalStorage
global, but I'm just trying to make sure it still works for Browserify and Electron which use the node-style module.exports
.
Thanks again for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it looks like your fix hasn't been pushed to your remote.
From your repo:
git push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed my changes with the typo fix
Am I being dumb and missing something? (I am rather new to Nodejs overall)
Here is what happens when I try to require from the root of my repo:
brad:angular-localForage$node
> var localForage = require('.')
ReferenceError: window is not defined
at /home/brad/Documents/GitHub/angular-localForage/dist/angular-localForage.js:11:44
at Object. (/home/brad/Documents/GitHub/angular-localForage/dist/angular-localForage.js:25:3)
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)
at Function.Module._load (module.js:438:3)
at Module.require (module.js:497:17)
at require (internal/module.js:20:19)
at repl:1:19
>
also tried
var localForage = require('angular-localforage')
same error message as above.
@bradtaniguchi
No worries. I mentioned that it might not be possible to require from node
since it doesn't have LocalStorage or IndexedDB or any of that. I'll test
it in Electron tonight and that should be enough.
…On Wed, Jan 4, 2017 at 1:51 PM Brad ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dist/angular-localForage.js
<#131>:
> @@ -14,7 +14,11 @@
return factory(angular, localforage);
});
} else if(typeof exports === 'object' || typeof global === 'object') {
- module.exports = factory(angular, require('localforage')); // Node/Browserify
+ if(typeof module === 'undefined') {
+ global.module.exports = factory(angular, require('localforage')); // NW.js
+ } else {
+ modules.exports = factory(angular, require('localforage')); // Node/Browserify
Pushed my changes with the typo fix
Am I being dumb and missing something? (I am rather new to Nodejs overall)
Here is what happens when I try to require from the root of my repo:
brad:angular-localForage$node
> var localForage = require('.')
ReferenceError: window is not defined
at
/home/brad/Documents/GitHub/angular-localForage/dist/angular-localForage.js:11:44
at Object.
(/home/brad/Documents/GitHub/angular-localForage/dist/angular-localForage.js:25:3)
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)
at Function.Module._load (module.js:438:3)
at Module.require (module.js:497:17)
at require (internal/module.js:20:19)
at repl:1:19
>
also tried
var localForage = require('angular-localforage')
same error message as above.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#131>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/ABmrErkJO3M3NvwteFrkWMmgUtfdEYcnks5rO-oygaJpZM4LZ7Iz>
.
|
Now the factory assignment for localforage is applied to
global.modules.exports
instead ofmodules.exports
ifmodule
isundefined.
module
is undefined in an NW.js environment (for whateverreason). So now we check for module and we uses global if
module
is undefined. Otherwise we proceed as before.
Thank @ashleyhindle for fix.
Fixes #77