update babylon to babel/parser 7.x #30
Conversation
|
@firsttris thanks for the PR, before diving into the code, here is a conversation over a year ago that you might find relevant. IMHO, the most uncertain part of this migration is the plugin list. We used to load all the plugin (yeah, that is a bit crazy), now with an explicit plugin list, I am not sure if we will encounter issues that the user plugin list != our parser's. Will that be an issue for parsing their code? If it does, do we need to consider allow users to pass in their plugin list or even reading their babel config? (a can of worms...) Thoughts? |
|
reading the users babel config sounds complex to me . big fan of kiss principle. if the parser supports more features that then user is using everything works well. if adding all plugins has worked before.. why not continue this way? i tried to add as many plugins as possible. some plugins:
they outputed this errors: i actually added them as an array of strings? also removed the typescript_parser as @stephtr suggested. |
reasonable starting point.
see discussion
should be... but we should probably create some fixtures with some of these features, such as optional-chaining, dynamic import etc, to make sure they can be parsed. |
There could be situations where Babel won't work with our configuration, but I'm quite sure that also the Babylon parser (as it is currently used) would have failed for them, it therefore wouldn't be a regression. |
There was a problem hiding this comment.
Hurrah! I think this is a big step in the right direction! Thank you! There's only one question that @connectdotz and/or @stephtr didn't seem to already cover - is the CHANGELOG.md OK like that? I'll approve it now so we're not stuck with me being sloooooooooow. 😪
You've already had the discussion about use plugins. We'll probably want a seam / way for the user to provide options, but we can add options to the parse functions in another PR.
|
Oh!! I am tired and forgot to ask about Yarn. Does Yarn update package-lock.json now too or did you use |
There was a problem hiding this comment.
@stephtr and @seanpoulter already covered many grounds, I will just add a few relatively simple requests. I think we are quite close to merging this!
P.S. I assume you are going to do the actual parser change for the test.each in a separate PR, this is just to switch over to the babel-7 world, right?
yes, in those enterprises they told to always keep my PR's small :) i don't wanted to extend two parsers. |
i think i used npm, as far as i know package-lock.json is only created by npm |
I wonder if we allow both yarn and npm lock files, will we encounter a situation that developers will end up with different versions of the dependencies based on their pm preference? Or otherwise we will ask developers to update both lock files by running boh yarn and npm? 🤔 Maybe it is best to standardize on the PM, which right now is yarn... |
There was a problem hiding this comment.
@firsttris lots of good change here 👍, a few notes:
- plugin customization: nice feature, but suggest we still have a solid default, more detail inline
- readme: great start, some minor comment inline, might need to be updated if plugin customization changed.
- npm support: personally I prefer us to simply standardize on
yarn. But if you would like to add npm support that would be fine as long as there is a mechanism to sync the 2 lock files automatically so they won't get out of sync.
src/parsers/babel_parser.js
Outdated
| export const plugins = ['jsx', 'classProperties']; | ||
|
|
||
| // Its not possible to use the parser with flow and typescript active at the same time | ||
| export const parseJs = (file: string, data: ?string, additionalPlugins: string[] = []): ParseResult => | ||
| parse(file, data, {plugins: [...plugins, ...additionalPlugins, 'flow']}); | ||
| export const parseTs = (file: string, data: ?string, additionalPlugins: string[] = []): ParseResult => | ||
| parse(file, data, {plugins: [...plugins, ...additionalPlugins, 'typescript']}); |
There was a problem hiding this comment.
Allow customization of the plugin is great 👍 , but I think I would make it more "optional" by providing a solid default set of plugins but allow override (customization might mean add or remove plugins). I think this way it provides the least interruption while adding flexibility.
There was a problem hiding this comment.
i provided a more solid list of default plugins. (for feature i see used more regularly).
With spreading those plugins together the consumer is able to add addtional plugins.
But i don't see why anyone would want to remove plugins, this would require a different implementation. maybe like: use the alternativePlugins instead of plugins if provided.
| parse('fileName.ts', codeTobeParsed, false, additionalPlugins); | ||
| expect(spy).toBeCalledWith(codeTobeParsed, parserConfigTypescript); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
not sure if I missed it but I didn't see any additional fixture for testing plugin specific syntax?
There was a problem hiding this comment.
no you didn't missed them, i have not added any yet.
i thought about this topic, normally you would only test your package, not the 3rd party depdenencies the project is using.
the plugin features should have already been tested in the babel project.
i mean we could not even fix a issue, if some tests were red, since its not within this project.
There was a problem hiding this comment.
I see where you are coming from. I think I am less concerned about "if babel implemented the plugin correctly" but "if the plugin list we passed in has the effect we anticipated"...
I played with the new babel-parser_plugin.test.js to try to answer my own question:
- comment out all additional plugins, it passed; ok, maybe all of our code fixures/snippets are just standard js that requires no additional plugin...?
- passed in a garbage plugin, it still passed. ok, maybe babel load plugin like a best-effort...?
- I then created some optional chaining code-snippet with empty additional plugin list, thinking for sure it should fail without passing the correct plugin, right? But surprise surprise, it still passed.
At this point, I feel there is something I don't understand or misunderstand about this plugins thing... maybe it is reading the babel.config.js locally? does that mean it will be impacted by users' env as well once we deployed, which might be good, or bad... How do tools like vscode-jest or yours decide what plugin list to pass in? If they can't decide then should all users now need to pass in their own list, even if they don't use babel? With the current implementation (minimal default), how disruptive the next release will be? Do we really need to get tangled with the rather complex user babel config/env?
Anyway, tests mean to surface issues, and I think without the simple additional tests I would not have discovered the unexpected behaviors and questions... in addition to tests, we should answer the questions above to make sure we fully understand what the community and users might face with these changes... thoughts?
There was a problem hiding this comment.
i don't have an answer to your questions yet but i appreciate that you are trying to understand it properly. so please don't get my comment wrong.
just one side thought, which might be interesting (in terms of testing)
majestic doesn't seem to take this topic that seriously:
https://github.com/Raathigesh/majestic/blob/master/server/services/ast/parser.ts#L10
they just use 3 plugins => "jsx", "classProperties", "optionalChaining"
There was a problem hiding this comment.
Hey @connectdotz. I had a look at this too and think that I've made some headway. Have a look at this babel documentation that I found helpful (I thought I knew what the plugins did by the names - I wasn't always right). That page will give you some useful snippets to test with later on.
Experimenting with babel-parser_plugin.test.js:
Hopefully I'm not pointing out the obvious, but this file is aimed at testing whether the correct plugin config is passed to the parser rather than whether that configured parser can parse the code correctly. You can see this by the fact that the "code" it is asked to parse is on line 26:
const codeTobeParsed = 'const bla = 1';which as you can see is pretty simple JS and is not testing any of the experimental language features that the plugins are supposed to be supporting. Which is probably why when you removed all the plugins, it still parsed correctly and the tests passed.
When I extended that line with snippets from the Babel documentation like this:
const codeTobeParsed = `
//classPrivateProperties
class A { #b = 1; }
//classPrivateMethods
class B { #c() {} }
//doExpressions
var a = do { if (true) { 'hi'; } };
`;I got test failures when the appropriate plugins were not enabled. Hope that helps.
There was a problem hiding this comment.
exacly @rossknudsen this test was only to verfiy if the addtionalPlugins have been merged with the default plugins correctly.
since i have added this feature i thought it make sense to test it.
There was a problem hiding this comment.
also did some research on this topic.
babel-parser_plugin.test.js is a good place to test the babel plugins.
i found that for example ObjectRestSpread works even if no plugin is activated. While classPrivateProperties or doExpressions do only work with plugins activated.
that is probably because some proposal are already on a later stage and work even without explicitly activating them.
ObjectRestSpread and OptionalChaining are already in stage 4.
does this answer your concerns @connectdotz ?
There was a problem hiding this comment.
@firsttris yes it does! thanks for doing the research 👍
I also dig into the babel-parser code today and found that it checked for only a small set (~11) of plugins, such as logicalAssignment, classProperties, and the document did mention
The latest ECMAScript version enabled by default (ES2017).
This is great and does eliminate my biggest concern. Obviously, as the language evolves the extra plugin list will too, but as long as we keep the babel-parser up to date we should be able to support the majority of the use cases.
There was a problem hiding this comment.
Another take away from babel-parser code: The logic to recognize and process these non-standard language features are already in the parser, the plugin strings are just like flags to allow processing the corresponding syntax if encountered in the content-to-parse.
For jest-editor-support, I don't see much downside to allow the parser to parse as much as it can. If the users choose to constraint it for their specific use case, which should be rare, they can certainly provide its plugin list to narrow and override the default.
there are 2 plugins require additional flags, if there is really no obvious winner (more commonly used), then whichever flag is fine, provided we document it so users know how to override it if need to.
There is no point to pass in "new" plugin because the babel-parser we packaged will not understand them anyway...
Co-Authored-By: Sean Poulter <sean.poulter+gh@gmail.com>
|
@firsttris yeah it's because the JS files have Flow typings in them. TS compiler doesn't understand them. Which is why we're using Babel to transpile, as it understands everything. So the tsconfig is really only being used to typecheck the TS files and we want it to ignore the JS files. |
|
hey @connectdotz & jest-community, i did my best to implement your suggestions and change requests. If you have additional change request please let me know. once i find time between my worktime, i will do my best to implement them. really want to continue extend that parser with test.each support, i see many projects heavely using this and our addons are not supporting this feature. From the user's perspective, that's a bug. |
|
hey @firsttris sorry for the long delay, was really busy this month, I will try to take a look this weekend, thanks for being patient. |
There was a problem hiding this comment.
hey @firsttris not sure if you are still interested in this PR or maybe we both are waiting for the other to take the next step...?
I see the major blocker is in how we invoke babel-parser, i.e. the options, as well as how we customize it... see the inline comments.
This change will most likely impact the README, tests, and maybe others... let me know when you are happy with the changes, I will do a final review then
Sorry for such a long delay, hope you haven't given up...
src/parsers/babel_parser.js
Outdated
| export const plugins = [ | ||
| 'jsx', | ||
| 'classProperties', | ||
| 'exportDefaultFrom', | ||
| 'logicalAssignment', | ||
| 'nullishCoalescingOperator', | ||
| 'objectRestSpread', | ||
| 'optionalChaining', | ||
| 'topLevelAwait', | ||
| ]; | ||
|
|
||
| // Its not possible to use the parser with flow and typescript active at the same time | ||
| export const parseJs = (file: string, data: ?string, additionalPlugins: string[] = []): ParseResult => | ||
| parse(file, data, {plugins: [...plugins, ...additionalPlugins, 'flow']}); | ||
| export const parseTs = (file: string, data: ?string, additionalPlugins: string[] = []): ParseResult => | ||
| parse(file, data, {plugins: [...plugins, ...additionalPlugins, 'typescript']}); |
There was a problem hiding this comment.
giving the plugins here are just "flags" for the babel-parser, I suggest we add all of them, you can reference their list here.
Once we enabled everything, there is no point to add "additional"... the customization should be very rare, if any, and it could be simply an override.
There was a problem hiding this comment.
it was not possible to add every plugin, but i added almost every plugin. i removed the ones which caused conflicts.
There was a problem hiding this comment.
i wonder if adding all those plugins is even necessary
|
hey @connectdotz to be honest is was not totally clear for me what tasks where missing to complete this PR. to summarize the open tasks are:
do you see any additional changes to be done @connectdotz ? |
|
@firsttris your summary is right, I will take another look when you are done... |
|
did it but travis now says The command "eval yarn --frozen-lockfile " failed 3 times. what can i do about it? |
|
This is the error: error semver@7.3.2: The engine "node" is incompatible with this module. Expected version ">=10". Got "8.17.0" I think you need to change the node version to 10 in the Travis config. I think I had to do that in my PR. |
|
thx @rossknudsen that worked. just coveralls is complaining now. |
There was a problem hiding this comment.
@firsttris thanks for the update, we are almost there! a few non-blocking document comments, and a question about the plugin list.
Don't worry about coverage complaint, we should be good to go once we resolve the issues below
| - filePath = Path to the file you want to parse. | ||
| - serializedData = Serialized data, will be used instead of the filePath if available (optional). | ||
| - strictMode = If this option is activated the parser throws an exception if the filetype is not detected, defaults to false. | ||
|
|
There was a problem hiding this comment.
great way to start documenting the API 👍 , can we throw in an example, I am a big fan of "learn-by-example'
There was a problem hiding this comment.
would be best to create an example with typescript and to link it in the readme (maybe follow up PR)
| export const plugins = [ | ||
| 'asyncGenerators', | ||
| 'bigInt', | ||
| 'classPrivateMethods', | ||
| 'classPrivateProperties', | ||
| 'classProperties', | ||
| 'doExpressions', | ||
| 'dynamicImport', | ||
| 'estree', | ||
| 'exportDefaultFrom', | ||
| 'exportNamespaceFrom', // deprecated | ||
| 'functionBind', | ||
| 'functionSent', | ||
| 'importMeta', | ||
| 'jsx', | ||
| 'logicalAssignment', | ||
| 'nullishCoalescingOperator', | ||
| 'numericSeparator', | ||
| 'objectRestSpread', | ||
| 'optionalCatchBinding', | ||
| 'optionalChaining', | ||
| 'partialApplication', | ||
| 'throwExpressions', | ||
| 'topLevelAwait', | ||
| ]; |
There was a problem hiding this comment.
looks like there is something missing from the babel plugin list, is it intentional?
There was a problem hiding this comment.
there are conflicts between various plugins, tried to include all and removed the ones which caused conflicts
|
hmm... on the 2nd thought, I think this is a good enough start, I can fix the remaining minor issues in another PR so we can free you to move on to the original thing you wanted to fix:
@firsttris if there is anything you want to add I can wait, otherwise I can merge it tonight. |
|
@connectdotz i'm rdy |
|
@firsttris thank you for your hard work 🙏 and sorry it took so long. |
|
Yay! Thank you Tristan! 🥇 |
|
@firsttris while testing the new typescript babel_parser, noticed a discrepancy vs the old typescript parser. While the old parser does not return the right "name" for the If you already started to work on the |
|
Have not yet started looking in to this topic. |
|
ok, I did some quick fix at #47, would love to have you review it. |
Hey jest-community,
i update babylon to babel/parser 7.x as suggested by @seanpoulter #29
now the typescriptt_parser.js is actually no longer relevant and could possibly be removed. also the typescript dependency in package.json.
since the babel_parser.js is now able to parse typescript as well.
i did not delete typescript_parser.js and corresponding test yet. (even if it would no longer be used after this PR)i add a addtional test - babel_typescript_parser.test.js which executes babel_parser in typescript mode.
i checked that all tests are green, and fixed a few eslint issues.
if you are happy with this PR, i would proceed update the jest parser to new features like test.each.
regards
Tristan