Use @babel/core#parse #594
Conversation
kaicataldo
force-pushed
the
babel-parse
branch
9 times, most recently
from
3fa21ee
to
483cd03
Compare
| @@ -36,7 +32,14 @@ | |||
| "url": "https://github.com/babel/babel-eslint/issues" | |||
| }, | |||
| "homepage": "https://github.com/babel/babel-eslint", | |||
| "peerDependencies": { | |||
There was a problem hiding this comment.
This is a lot of peer deps; eslint and babel-core makes sense, but aren’t the other two addressed by babel-core?
There was a problem hiding this comment.
Yeah code-frame and babylon are dependencies of babel/core that's true. So if you are on npm 3 it should be fine?
There was a problem hiding this comment.
Since we rely on those packages directly and they could potentially be removed from @babel/core's dependencies I thought it would be safer this way. Can definitely remove them if we think that's being overprotective!
There was a problem hiding this comment.
The goal is to set up the deps such that when the user runs npm install && npm ls, everything exits zero.
Since we rely on them directly, I'd rather them be deps. However we could make them both deps and peerDeps, and then anyone on npm3+ should get them autoinstalled and satisfy the peer dependency, and any conflicts due to changes in babel-core would cause npm ls to fail.
package.json
Outdated
| "devDependencies": { | ||
| "@babel/core": "7.0.0-beta.40", |
There was a problem hiding this comment.
The devDep range should be identical to the peerDep range.
lib/parse.js
Outdated
| }; | ||
| var enableCodeFrame = options.hasOwnProperty("codeFrame") ? options.codeFrame : true; |
There was a problem hiding this comment.
This might break if i pass a “hasOwnProperty” option; you probably want Object.prototype.hasOwnProperty.call or the has module (i realize this isn’t related to your PR, but it’s an easy fix)
lib/parse.js
Outdated
| "pipelineOperator", | ||
| "nullishCoalescingOperator", | ||
| ], | ||
| filename: (options.filePath && options.filePath !== ESLINT_DEFAULT_FILEPATH) ? options.filePath : '', |
There was a problem hiding this comment.
I'd say pass undefined if there is no filename.
lib/parse.js
Outdated
| err.message.replace(/ \((\d+):(\d+)\)$/, "") + | ||
| // add codeframe | ||
| "\n\n" + | ||
| codeFrameColumns( |
There was a problem hiding this comment.
Doesn't .parse throw with an included code frame now?
lib/parse.js
Outdated
| @@ -1,87 +1,73 @@ | |||
| "use strict"; | |||
|
|
|||
| var babylonToEspree = require("./babylon-to-espree"); | |||
| var parse = require("babylon").parse; | |||
| var tt = require("babylon").tokTypes; | |||
There was a problem hiding this comment.
Should we consider exporting the babylon module namespace from babel-core? The only reason not to is if we were going to version them independently, and realistically that doesn't seem likely.
There was a problem hiding this comment.
Then we could ditch the babylon dependency and not risk them being out of sync, right?
|
👋Is there anything the user community can do to support this work? I'd love to see TypeScript support land, so I'd be happy to volunteer some nights and/or weekends if I can be useful. |
|
@phyllisstein TypeScript support is a separate issue than this PR solves. Hoping I can pick this back up soon - I've been busy with work and just haven't had time. |
|
Any developments on this? What’s left to be done? I’m also happy to help if I can. |
|
Sorry for the hold up. Hoping I can find time for this in the next few weeks. |
kaicataldo
force-pushed
the
babel-parse
branch
3 times, most recently
from
dfad9e5
to
7a3a425
Compare
|
@kaicataldo - with the recent approval from @sibelius is there a chance we can resolve the conflicts for merge? It would be great to use this feature. |
|
@dan-kez This PR wasn't quite complete - I still hadn't figured out how to get this tested. I'm hoping I can have some time soon (apologies to everyone for the delay), but with the Babel 7 release I'm wondering if it would actually be better just to close this and start over with the implementation. |
|
Closing this in favor of starting a new PR. Will start looking at it tonight. |
|
New PR open here: #711 |
fixes #573
This is a WIP. I still need to figure out how to test this.
@babel/babel Would love input on if this seems like a good way to go, behavior-wise.