-
Notifications
You must be signed in to change notification settings - Fork 101
Support for importing js files and modules via require #5
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
base: master
Are you sure you want to change the base?
Conversation
newContext[@"require"] = self.jsContext[@"require"]; | ||
newContext[@"process"] = self.jsContext[@"process"]; | ||
newContext[@"console"] = self.jsContext[@"console"]; | ||
[newContext evaluateScript:@"var exports = {};"]; // Evaluated so the developer doesnt have to |
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.
First, great work. This is a big step forwards.
To mimic Node.js behavior you should inject a module
object that has an exports
object initialized as an empty object. Pseudo-code:
module = @{ @"exports": @{} }
newContext[@"module"] = module;
newContext[@"exports"] = module.exports;
// 1.- evaluate
// 2.- pick the evaluated exports
return (id)[tmpContext objectForKeyedSubscript:@"module.exports"];
This way these two ways of populating exports
will work:
exports.something = foo
module.exports = somethingCompletelyNew
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.
Sounds good! Just a node I originally used exports
due to this article.
I've done a bit more research on this, and it does seem a bit confusing about the whole exports
vs module.exports
.
Regarding your suggested changes, how would setting exports
also change module.exports
? Unless I'm missing something, we are only exposing module
, unless you propose that we leave both, and we check which variable is populated...
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.
Sorry, my mistake. It should be
newContext[@"exports"] = module.exports;
I've changed it in the example. I'll try to help more on this later.
I've created this PR: https://github.com/ninjaprawn/electrino/pull/1/files That adds better support. Nevertheless the whole thing should follow this documentation: Some things that we will need to implement:
|
Better support for "module" and "exports"
I think that once we've established where the project is heading, we will add those features in a reliable way. |
Hi @ninjaprawn @gimenete @kittsuphatt ! Thanks very much for this. (I failed at Github and just didn't notice this pull request until now.) I'll take a look and merge ASAP. |
@ninjaprawn would be awesome if you can fix the merge conflicts. I can merge this after testing it out |
Oh man -- when I wrote "ASAP", I didn't mean to imply "20 days later". Sorry for that. I've just been swamped. I can check out and fix the merge conflicts on this one tomorrow, I think. |
I won't be able to do much for the next couple of days (I'm flying back from WWDC to Australia, which is a long time). If nothing is done until then, I'll be more than happy to fix the conflicts |
Clean up JSBrowserWindowInstance constructor
What ever happened to this project? |
@ninjaprawn is this PR ready to review? |
@ninjaprawn please help with the merge conflicts! |
sorry this took so long! should be good to go now |
@pojala let's move ahead with this PR! 😄 |
this proj is still active? |
@gimenete Should this be closed since your PR was merged? |
@amilajack my PR improved things, but was not implementing everything. I'm unsure about the state of the current implementation. The implementation should follow this spec https://nodejs.org/api/modules.html#modules_all_together |
@ninjaprawn Would love if you could the merge conflicts one last time. I'll definitely merge this right after you fix them. |
@ninjaprawn could you help us out with the merge conflicts - we are looking to merge this in. |
Modules are a key aspect in creating an application. In Node.js, modules can be installed and imported.
This PR adds support for importing js files, as well as modules that could come from Node packages.
What this PR does:
require
implementationJSContext
which is a duplicate of the default one-copyWithZone:
methodtest-app
folder toapp
app
group to a folder reference, so that files can be easily copied without having to manually add them to aCopy Files
phase.If you have any questions, feel free to ask :)