-
Notifications
You must be signed in to change notification settings - Fork 111
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
support user .babelrc #347
Conversation
…ment transform in react preset
this is handled as an option in kyt core
… be used with kyt so they should support older versions of node
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 is really great! Just a couple tiny questions but otherwise looks good. We should document this at the top level and maybe in the starter-kyts to explain how the preset works?
"babel-preset-kyt-core": "0.1.0-alpha.1", | ||
"babel-preset-react": "6.16.0" | ||
}, | ||
"keywords": [ |
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 do this for our other packages?
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.
probably!
const hasBabelrc = shell.test('-f', userBabelrcPath); | ||
if (!hasBabelrc) { | ||
logger.warn('No user .babelrc found. Using kyt default babel preset...'); | ||
} |
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.
Just checking these are the only changes you made in this file yah? The rest of the diff just looks like spacing
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.
yeah, I had to indent everything because it just immediately returned an object before
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.
👍 🌮
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.
Couple questions but otherwise LGTM 👍
@@ -0,0 +1,5 @@ | |||
{ | |||
"rules": { | |||
"no-var": 0 |
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.
How come?
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.
I'm also curious of the argument for using < ES2015 in the presets.
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.
maybe this is naive but I figured there's nothing about these presets that really requires them to be used inside kyt, so might as well make them es5 compatible
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.
FWIW these .eslintrc.json
s aren't even honored right now since the root package's lint
ignores them (to work around needing to get the eslint config cascade working)
const createBabelrc = () => { | ||
// back up existing .babelrc, if it exists | ||
if (shell.test('-f', userBabelrcPath)) { | ||
const mvTo = path.join(userRootPath, `.babelrc-${date}.bak`); |
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.
Maybe append a time stamp so there's no collisions
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.
date
is set to Date.now()
above so I think we should be ok here?
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 sorry missed the date
bit, yeap perfect 👍
if (dep.indexOf('babel') !== 0) { | ||
try { | ||
resolved = resolve.sync(`${prefix}-${dep}`, { basedir: userRootPath }); | ||
} catch (e) { /* eslint-disable no-empty */ } |
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.
What does this error case mean?
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.
resolve.sync
throws if you try to resolve something that doesn't exist. maybe we should log the error?
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.
I think so right? Because this would be an exceptional case, where there's a babel plugin referenced that doesn't exist so it should fail loudly, I think.
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.
@ccpricenytimes agreed that this needs docs--should I do it here, or in the 0.4.0 docs branch? |
fixes #322