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

[TIMOB-16078] Implement full NodeJS require specs so we can use NPM for modules #8075

Closed
wants to merge 4 commits into from

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Jun 20, 2016

JIRA: https://jira.appcelerator.org/browse/TIMOB-16078

Description:
This adds searching node_modules directories up the directory tree for modules when doing a require without any /, ./, or ../ prefix.

Continuation of #8004 work.

TODO: What's the priority between node modules, CommonJS modules, and legacy Titanium requires? Currently this will try to load module.id/module.id.js for CommonJS first, then node_modules, then legacy/CommonJS loading.

So with this PR, the require algorithm looks like this:

require(X) from module at path Y
1. If X is a core module,
   a. return the core module
   b. STOP
2. If X begins with './' or '../'
   a. LOAD_AS_FILE(Y + X)
   b. LOAD_AS_DIRECTORY(Y + X)
3. If X begins with '/'
   a. LOAD_AS_FILE(X)
   b. LOAD_AS_DIRECTORY(X)
4. If X **does not** contain '/' # Assume we should try and load CommonJS module first...
   a. LOAD_AS_FILE(X/X.js) # Try to load "legacy" CommonJS file named module.id/module.id.js
   b. LOAD_AS_DIRECTORY(X) # try to load CommonJS module as a directory
5. LOAD_NODE_MODULES(X, dirname(Y))
6. WARN user about possibly bad require being treated as absolute 
   a. LOAD_AS_FILE(X)
   b. LOAD_AS_DIRECTORY(X)
7. THROW "not found"

LOAD_AS_FILE(X)
1. If X is a file, load X as JavaScript text or JavaScript Object (if extension is json).  STOP
2. If X.js is a file, load X.js as JavaScript text.  STOP
3. If X.json is a file, parse X.json to a JavaScript Object.  STOP

LOAD_AS_DIRECTORY(X)
1. If X/package.json is a file,
   a. Parse X/package.json, and look for "main" field.
   b. let M = X + (json main field)
   c. LOAD_AS_FILE(M)
2. If X/index.js is a file, load X/index.js as JavaScript text.  STOP
3. If X/index.json is a file, parse X/index.json to a JavaScript object. STOP

LOAD_NODE_MODULES(X, START)
1. let DIRS=NODE_MODULES_PATHS(START)
2. for each DIR in DIRS:
   a. LOAD_AS_FILE(DIR/X)
   b. LOAD_AS_DIRECTORY(DIR/X)

NODE_MODULES_PATHS(START)
1. let PARTS = path split(START)
2. let I = count of PARTS - 1
3. let DIRS = []
4. while I >= 0,
   a. if PARTS[I] = "node_modules" CONTINUE
   c. DIR = path join(PARTS[0 .. I] + "node_modules")
   b. DIRS = DIRS + DIR
   c. let I = I - 1
5. return DIRS

@sgtcoolguy
Copy link
Contributor Author

cc @jawa9000 This PR will extend the behavior changed in #8004 slightly, but should probably be folded into whatever docs you may have prepared for that. The addition here is the search for node_modules (step 5)

@@ -770,7 +770,7 @@ -(id)loadCommonJSModule:(NSString*)code withSourceURL:(NSURL *)sourceURL
* "/Resources/views/login/window.js" __dirname = "views/login"
*/

NSString *filename = [sourceURL lastPathComponent];
NSString *filename = [sourceURL lastPathComponent]; // FIXME This should be the absolute path to the file!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgtcoolguy isn't the absolute file path the following line?

NSString *js = [[NSString alloc] initWithFormat:TitaniumModuleRequireFormat, dirname, filename,code];

What do you need changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a separate ticket for parity about this, and didn't really investigate it in this one. Basically __filename should be the full absolute path to the resolved file on all platforms. On iOS and Android we report just the filename (i.e "test.js"). On Windows we report the base name of the file (i.e. "test")

See https://jira.appcelerator.org/browse/TIMOB-23543

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in this case, it's some variation of the sourceURL without the preceding scheme and maybe some of the first path segments we don't "expose" (like maybe a "Resources/" prefix)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgtcoolguy so now with this fixed here: #8098
do we still need to address this? @hansemannn fyi.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed with the mentioned PR. The line is only used to expose the __filename and that's done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k. @sgtcoolguy can you fix the merge conflicts? Thanks.

@sgtcoolguy sgtcoolguy added this to the 6.0.0 milestone Jun 30, 2016
@sgtcoolguy
Copy link
Contributor Author

@cheekiatng Ok, rebased on master and fixed the conflict.

@@ -1124,6 +1173,14 @@ - (id)require:(KrollContext *)kroll path:(NSString *)path
return module;
}
}

// Need base path to work from for determining the node_modules search paths.
NSString *workingPath = [oldURL relativePath];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct this indentation

@cheekiatng
Copy link
Contributor

cheekiatng commented Jul 7, 2016

FT passed. @sgtcoolguy Can you fix that minor indention and I'll be able to merge this.

@FokkeZB
Copy link
Contributor

FokkeZB commented Jul 7, 2016

👏🙌👏🙌👏🙌

@sgtcoolguy
Copy link
Contributor Author

Merged this manually after massaging the commits a little to combine the iOS changes into one, Android into one, unit test as one.

@sgtcoolguy sgtcoolguy closed this Jul 20, 2016
@sgtcoolguy sgtcoolguy deleted the TIMOB-16078 branch July 20, 2016 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants