Skip to content

Set the start dir of looking for tsconfig to process.cwd()#348

Merged
kulshekhar merged 4 commits intokulshekhar:masterfrom
Bnaya:start-dir-cwd
Oct 17, 2017
Merged

Set the start dir of looking for tsconfig to process.cwd()#348
kulshekhar merged 4 commits intokulshekhar:masterfrom
Bnaya:start-dir-cwd

Conversation

@Bnaya
Copy link
Copy Markdown
Contributor Author

Bnaya commented Oct 17, 2017

The failed test is:

 FAIL  tests/__tests__/synthetic-default-imports.spec.ts
  ● synthetic default imports › should not work when the compiler option is false
    expect(string).toContain(value)
    
    Expected string:
      "FAIL __tests__/module.test.ts
      the module which has no default export
        ✕ should return funky junk when trying to access its exports (113ms)
    
      ● the module which has no default export › should return funky junk when trying to access its exports
    
        TypeError: Cannot read property 'someExport' of undefined
          
          at Object.<anonymous> (__tests__/module.test.ts:6:16)
              at new Promise (<anonymous>)
              at <anonymous>
    
    Test Suites: 1 failed, 1 total
    Tests:       1 failed, 1 total
    Snapshots:   0 total
    Time:        1.619s
    Ran all test suites.
    "
    To contain value:
      "module.test.ts:6:15"

module.test.ts:6:16 and not module.test.ts:6:15

No idea why... should i just change the expected value?

Comment thread src/utils.ts
const grandparent = path.resolve(__dirname, '..', '..');
if (grandparent.endsWith('/node_modules')) {
return path.resolve(grandparent, '..');
return process.cwd();
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If we're using process.cwd(), there's no need for the getStartDir function at all. process.cwd() is synonymous with . so the conditions in the function don't make any sense.

It would be better to remove this function and replace all calls to getStartDir with process.cwd(). Alternatively, just replace the contents of getStartDir with

return process.cwd()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it seems like that to me but,
When i change the function to just return process.cwd()
more test fails

I assume it because absolute and relative paths are handled differently in some point in the code.
If its important i can try to figure what's going on.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're right. This change is good in its current form.

@kulshekhar
Copy link
Copy Markdown
Owner

No idea why... should i just change the expected value?

I'm not sure what's happening because this is failing only for Node v8 and just on Linux. I'll have to take a closer look. I'll also have to take a closer look at how this affects the fix for lerna projects ( #342 )

@kulshekhar
Copy link
Copy Markdown
Owner

@Bnaya just some minor things

  • add your name to the AUTHORS file (in alphabetical order)
  • bump the version patch in package.json so that this can be published to npm right away

@kulshekhar kulshekhar dismissed their stale review October 17, 2017 13:26

The change was correct

@Bnaya
Copy link
Copy Markdown
Contributor Author

Bnaya commented Oct 17, 2017

What about the failing test?

@kulshekhar
Copy link
Copy Markdown
Owner

I've pushed a change for that

@Bnaya Bnaya force-pushed the start-dir-cwd branch 2 times, most recently from fe4333a to c19b7e1 Compare October 17, 2017 13:46
@Bnaya
Copy link
Copy Markdown
Contributor Author

Bnaya commented Oct 17, 2017

Done,
No idea why appveyor is failing :X

@kulshekhar
Copy link
Copy Markdown
Owner

I've rerun the test on appveyor and it looks like it'll pass now. I have no idea what happened.

@kulshekhar kulshekhar merged commit 0643b03 into kulshekhar:master Oct 17, 2017
@kulshekhar
Copy link
Copy Markdown
Owner

This has been published.

Thanks @Bnaya :)

@Bnaya
Copy link
Copy Markdown
Contributor Author

Bnaya commented Oct 17, 2017

Thank you for this great tool!!

@Bnaya Bnaya deleted the start-dir-cwd branch October 17, 2017 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ts-jest is failing to locate the tsconfig when being run from inside yarn workspace (or lerna)

2 participants