fix: check pkg main #797

calebboyd · 2017-09-14T05:07:32Z

This correctly identifies the main file (afaict)

But its still not resolving to the correct module. (null at runtime), so some tests are probably broken

nchanged · 2017-09-14T07:04:34Z

Seems like a legit fix to me, let's figure out what's wrong with tests, we need to make them pass

calebboyd · 2017-09-14T14:20:48Z

linking: #795

calebboyd · 2017-09-14T15:13:04Z

So this fix doesn't work.. It works in the context of "testFolder" But the require statement is not resolving the correct file. If you can point me in the right direction I can continue to investigate, but Im not very familiar with this portion of fusebox

nchanged · 2017-09-14T16:04:05Z

well it should be here

fuse-box/src/core/PathMaster.ts

Lines 233 to 246 in 91b4c47

    
           if (fs.existsSync(folder)) { 
        
               for (let i = 0; i < extensions.length; i++) { 
        
                   let ext = extensions[i]; 
        
                   const index = `index.${ext}`; 
        
                   const target = path.join(folder, index); 
        
                   if (fs.existsSync(target)) { 
        
                       let result = path.join(name, index); 
        
                       let startsWithDot = result[0] === "."; // After transformation we need to bring the dot back 
        
                       if (startsWithDot) { 
        
                           result = `./${result}`; 
        
                       } 
        
                       return result; 
        
                   } 
        
               }

That's for sure, and has to return alias:

fuse-box/src/core/PathMaster.ts

Lines 296 to 299 in 91b4c47

    
           return { 
        
               resolved: path.resolve(root, name, folderJSON.main), 
        
               alias: this.ensureNodeModuleExtension(name) 
        
           }

Actually, alias should come here:

fuse-box/src/core/PathMaster.ts

Lines 311 to 313 in 91b4c47

    
           const folderPath = this.testFolder(folder, name); 
        
           if (folderPath) { 
        
               return { resolved: folderPath }

calebboyd · 2017-09-15T06:35:28Z

I'm out for the night. I think I've fixed the issue. Please review!

nchanged · 2017-09-15T07:01:06Z

gulpfile.js

@@ -326,12 +326,8 @@ gulp.task("installDevDeps", function(done) {
        "buble",
    ];

-    if (os.platform().match(/^win/)) {
-        let windowsCommands = ["start", "cmd.exe", "/K", "npm", "install", "--no-save", ...deps];


why did you remove that one?

I replaced it with spawn('npm.cmd', ...) on windows

This avoids a shell popping up, that has to be closed manually before the task will finish

nchanged · 2017-09-15T07:02:31Z

src/loader/LoaderAPI.ts

        validPath = $pathJoin(filePath, "/", "index.js");
        file = pkg.f[validPath];
+
+        if (!file && pkg.s && pkg.s.entry) {


what's that? Why did you modify the loader?

If the path is fully relative it will land here. See:
https://github.com/calebboyd/fuse-test

If its not the index it must be an entry file. Specifically, a fully relative name.

'..' or '../../'. As a guard an isRelative check could be made..

Thoughts?

But what if there a package.json with a different entry? that's why we had that alias thingy in PathMaster, that overrides it

If you have an idea of what alias needs to be set to, any tips would be helpful.

From what I can tell providing an alias does nothing -- The property is set but then it is never used again...
Is that correct?

fuse-box/src/core/PathMaster.ts

Lines 153 to 155 in 813bc79

if (absPath.alias) {

data.fuseBoxAlias = absPath.alias;

}

calebboyd · 2017-09-16T04:42:54Z

src/core/PathMaster.ts

@@ -328,7 +328,7 @@ export class PathMaster {
                let folder = path.isAbsolute(name) ? name : path.join(root, name);
                const folderPath = this.testFolder(folder, name);
                if (folderPath) {
-                    return { resolved: folderPath }
+                    return { resolved: folderPath, alias: name }


Adding alias here doesn't really do anything

nchanged · 2017-09-16T07:08:04Z

src/core/PathMaster.ts

+            let result
+            const pkg = this.tryReadPkg(folder)
+            if (pkg && pkg.main) {
+                result = path.join(name, pkg.main)


This should return an alias (FuseBox will regenerate the code and point to a correct file)

nchanged · 2017-09-16T16:36:56Z

src/loader/LoaderAPI.ts

        validPath = $pathJoin(filePath, "/", "index.js");
        file = pkg.f[validPath];
+
+        if (!file && filePath === ".") {


Do we still need to modify the loader? Which case does it cover? require(".")?
Shouldn't it require index if exists?

Yeah, So here is an example of it being hit
https://github.com/calebboyd/fuse-test

Adding a log in that if block (L301)
console.log('loading: "', name, '" filePath: "', filePath, '" entry: "', pkg.s.entry, '"')

->

loading: " ../ " filePath: " . " entry: " test.js " loading: " ../../ " filePath: " . " entry: " test.js "

It covers any fully relative path that resolves to the package.json in the root of the package (".")

nchanged · 2017-09-16T16:37:45Z

Amazing, almost there, let's verify a few things before merging

dgreif · 2017-09-22T15:28:22Z

Bundled a new project with fusebox today. It uses request-promise, which does some interesting require paths in require-promise-core. It failed with 2.2.31, but worked with 2.3.1-beta.14. This is a real-world case where @calebboyd's fix is needed.

nchanged reviewed Sep 15, 2017

View reviewed changes

calebboyd force-pushed the resolve-pkg branch 2 times, most recently from fb01ea7 to ce8fd28 Compare September 15, 2017 18:59

calebboyd commented Sep 16, 2017

View reviewed changes

calebboyd force-pushed the resolve-pkg branch from e082f25 to aa42edc Compare September 16, 2017 05:12

nchanged reviewed Sep 16, 2017

View reviewed changes

calebboyd added 3 commits September 16, 2017 10:28

fix: check pkg main

bfd2e6f

fix: load entry if present

958bccf

refactor: assert base filepath before loading entry

58a0e5a

calebboyd force-pushed the resolve-pkg branch 2 times, most recently from 4aa79c4 to 424a01a Compare September 16, 2017 16:14

fix: don't append js to fully relative path

a5e9ede

calebboyd force-pushed the resolve-pkg branch from 424a01a to a5e9ede Compare September 16, 2017 16:15

nchanged reviewed Sep 16, 2017

View reviewed changes

nchanged approved these changes Sep 16, 2017

View reviewed changes

nchanged merged commit 3c219de into fuse-box:master Sep 16, 2017

calebboyd deleted the resolve-pkg branch September 16, 2017 17:28

dgreif mentioned this pull request Sep 25, 2017

Cannot import request-promise #823

Closed

calebboyd mentioned this pull request Oct 2, 2017

Changelog #839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check pkg main #797

fix: check pkg main #797

calebboyd commented Sep 14, 2017 •

edited

nchanged commented Sep 14, 2017

calebboyd commented Sep 14, 2017

calebboyd commented Sep 14, 2017 •

edited

nchanged commented Sep 14, 2017

calebboyd commented Sep 15, 2017

nchanged Sep 15, 2017

calebboyd Sep 15, 2017 •

edited

nchanged Sep 15, 2017

calebboyd Sep 15, 2017

nchanged Sep 15, 2017

calebboyd Sep 16, 2017

calebboyd Sep 16, 2017

calebboyd Sep 16, 2017

nchanged Sep 16, 2017

nchanged Sep 16, 2017

calebboyd Sep 16, 2017

calebboyd Sep 16, 2017 •

edited

nchanged commented Sep 16, 2017

dgreif commented Sep 22, 2017

fix: check pkg main #797

fix: check pkg main #797

Conversation

calebboyd commented Sep 14, 2017 • edited

nchanged commented Sep 14, 2017

calebboyd commented Sep 14, 2017

calebboyd commented Sep 14, 2017 • edited

nchanged commented Sep 14, 2017

calebboyd commented Sep 15, 2017

Choose a reason for hiding this comment

calebboyd Sep 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebboyd Sep 16, 2017 • edited

Choose a reason for hiding this comment

nchanged commented Sep 16, 2017

dgreif commented Sep 22, 2017

calebboyd commented Sep 14, 2017 •

edited

calebboyd commented Sep 14, 2017 •

edited

calebboyd Sep 15, 2017 •

edited

calebboyd Sep 16, 2017 •

edited