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.

Sign up for GitHub

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

Jump to bottom

Fix/find project root provides wrong root package json path #162

Merged
merged 4 commits into from
Jul 8, 2021
Merged

Fix/find project root provides wrong root package json path #162

merged 4 commits into from
Jul 8, 2021

Conversation

tonyt-adept
Copy link
Contributor

PR add's some fluff for the singular Jest test... More than happy to rip that out before final Merge, just wanted to showcase the error before/after.

Also regarding the fix - I tried to leave the original findUp logic intact by queuing off of string vs. string[] however I understand if you guys would like to go down another path to keep things a bit more clear & concise in there! Just let me know the guidelines and I'm happy to refactor.

Thanks again!

@tonyt-adept tonyt-adept mentioned this pull request Jul 7, 2021
Closed
@olup
Copy link
Contributor

olup commented Jul 7, 2021

Hey, actually I am sure everyone is thankful for some tests, so cheers for adding this one and fixing a bug there. I haven't checked the pr yet, but from a first look, just wondering if babel is needed here. Can't we just use ts-jest ?

@tonyt-adept
Copy link
Contributor Author

TBH my Typescript/Jest knowledge is fresh, so probably. I'll look into that and see if we can trim some of that fat!

@tonyt-adept
Copy link
Contributor Author

Poking around more - the Babel use comes right from the Jest Docs: https://jestjs.io/docs/getting-started#using-typescript

Seems there are some floating SO posts regarding how to bypass it, but it may be sketchier or less stable that just adding the extra babel dependencies for now. Just my opinion though!

@tonyt-adept tonyt-adept closed this Jul 7, 2021
@tonyt-adept tonyt-adept reopened this Jul 7, 2021
@olup
Copy link
Contributor

olup commented Jul 7, 2021

https://github.com/kulshekhar/ts-jest

@floydspace
Copy link
Owner

Hey @tonyt-adept , about tests, what you need is to add preset: 'ts-jest' in your jest.config.js, and then you can remove babel.
ts-node is already installed and even ci/cd configured to test, so what you need it just write tests.
Good work, thank you.

floydspace
floydspace reviewed Jul 7, 2021
View reviewed changes
jest.config.js Outdated Show resolved Hide resolved
floydspace
floydspace reviewed Jul 7, 2021
View reviewed changes
package.json Outdated Show resolved Hide resolved
@tonyt-adept
Copy link
Contributor Author

Thanks for laying it out guys! Appreciate the assist!

Hope she's lookin good now. I did prepublishPackage and sym-linked locally to confirm it resolved my issue without blowing things up. Hopefully it stays that way!

floydspace
floydspace approved these changes Jul 8, 2021
View reviewed changes
Copy link
Owner

@floydspace floydspace left a comment

Choose a reason for hiding this comment

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

Thank you @tonyt-adept , looks perfect to me. let me merge it.
And welcome to contribute more.

@floydspace floydspace merged commit e686244 into floydspace:master Jul 8, 2021
@github-actions
Copy link

github-actions bot commented Jul 8, 2021

🎉 This PR is included in version 1.16.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tonyt-adept tonyt-adept deleted the fix/findProjectRoot-provides-wrong-rootPackageJsonPath branch July 8, 2021 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floydspace floydspace floydspace approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tonyt-adept @olup @floydspace