-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
feat: Adds a resolved path for output #80
Conversation
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.
LGTM in general. See my comments.
lib/transformations/utils.test.js
Outdated
describe('getRequire', () => { | ||
it('should create a require statement', () => { | ||
const require = utils.getRequire(j, 'filesys', 'fs'); | ||
expect(require).toMatchSnapshot(); |
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.
This should probably create snapshot for the source code. Right now it's for AST.
expect(require.toSource()).toMatchSnapshot()
?
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.
Fragments dont actually have .toSource()
on them. :( Will try to figure out if this is possible some other way. I think I should be able to convert it to a node somehow.
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.
Had to wrap it in a j()
const path = require('path'); | ||
module.exports = { | ||
output: { | ||
path: path.join(__dirname, 'dist') |
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.
Should we add a test where path
is imported as a variable with a different name?
const p = require('path')
should also work I guess.
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.
Oh yeah, what is path is required in the file but output
doesnnt use it? Need to add some more checks.
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.
Added this.
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.
Almost there! Awesome work @pksjce 👏
}, false)); | ||
if (pathDecalaration) { | ||
isPathPresent = true; | ||
pathDecalaration.forEach(p => {pathVarName = utils.safeTraverse(p, ['value', 'id', 'name']);}); |
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.
Let's put the block code on a new line please. This is hard to read now.
isPathPresent = true; | ||
pathDecalaration.forEach(p => {pathVarName = utils.safeTraverse(p, ['value', 'id', 'name']);}); | ||
} | ||
console.log(pathVarName); |
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.
Let's remove console.log
calls please.
pathDecalaration.forEach(p => {pathVarName = utils.safeTraverse(p, ['value', 'id', 'name']);}); | ||
} | ||
console.log(pathVarName); | ||
literalOutputPath.find(j.Literal) |
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.
Should the .find()
be also a new line?
Hey @okonet - Thanks for the detailed review. I've taken care of the changes. |
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.
👍
Feel free to squash & merge with the commitizen-compatible commit message! |
* feat: Adds a resolved path for output * fix * fix: Add review fix * fix: test for source not ast * fix: review comments
What kind of change does this PR introduce?
This tries to address #73
Did you add tests for your changes?
Yep
If relevant, did you update the documentation?
NA
Summary
To summarise, it tries to implement what was promised in #73