-
Notifications
You must be signed in to change notification settings - Fork 129
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 passing babel plugin configuration in JSON #88
Conversation
Hi @byara and team - any chance you could let me know if you will be looking into including this (short) PR, or should I switch to working off a fork? |
Thank you @saaryab for the PR. We are looking into the PR and will release a new version very soon. |
src/preprocessor.ts
Outdated
|
||
const importNodes: ImportDeclaration[] = []; | ||
|
||
const parserOptions: ParserOptions = { | ||
sourceType: 'module', | ||
plugins: [...plugins, ...experimentalBabelParserPluginsList], | ||
plugins: [...plugins, ...experimentalParserPlugins], |
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.
Can you keep the word babel
in the name so it matches the config variable 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.
sure
// Some experimental plugins have configurations so they are passed as JSON | ||
// in the form of ["plugin-name", { configuration: true }] | ||
return experimentalBabelParserPluginsList.map( | ||
pluginName => pluginName.startsWith("[") ? JSON.parse(pluginName) : pluginName |
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.
- Are you sure it's not supported to use non-string arrays there? It seems weird to me that prettier validates types of config vars it isn't aware of.
What exactly happens when you put this in the YAML file?
experimentalBabelParserPluginsList:
- pluginA
- "pluginB"
- ["pluginC", { option: 1 }]
- If you do stick to the string items, consider creating a
tryParseJson(jsonEncoded)
function that returns the parsed JSON, or undefined if invalid, and then do:
pluginName => pluginName.startsWith("[") ? JSON.parse(pluginName) : pluginName | |
pluginName => tryParseJson(pluginName) ?? pluginName, |
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.
- Prettier requires plugins like this one to declare the options fields they add in
options
inindex.ts
. The types available are only'int' | 'boolean' | 'choice' | 'path'
(source), and an additionalarray
flag. Prettier then does it's own run-time type validation on the options and prints this error if I send an array in theexperimentalBabelParserPluginsList
:
[error] Invalid experimentalBabelParserPluginsList value. Expected an array of a string, but received [["decorators", { decoratorsBeforeExport: true }]].
error Command failed with exit code 1.
- I'll try making it more clear
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.
- Bahh that sucks :/ So your JSON solution sounds good
- Cool
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 kept the JSON parsing inline since that is the main purpose of this util, but I made the code more verbose so it would be more clear when and why we try to parse JSON and I added a clear error message in the case that the JSON is invalid.
return experimentalBabelParserPluginsList.map(pluginNameOrJson => { | ||
// ParserPlugin can be either a string or and array of [name: string, options: object] | ||
// in prettier options the array will be sent in a JSON string | ||
const isParserPluginWithOptions = pluginNameOrJson.startsWith("["); |
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.
The change I suggested was to remove this line - Instead of heuristically check if it's a JSON (like you do here), always try to parse as JSON.
Or another option that is less heuristical - you can detect plugin names using a regex (maybe `/^@?[a-zA-Z_0-9-]+(/[a-zA-Z_0-9-]+)?$/), and if it doesn't match, assume it's a JSON and then try to parse it.
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.
BTW, what about some short documentation? The JSON string solution isn't something that users would be able to guess, to you should update the README file
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.
Since the options only support either strings or arrays of [string, object]
I think this detection is correct and not just a heuristic.
Good catch on the documentation - done.
Hey @saaryab, I took a look at the PR and tried to test it on a sample project and by running prettier with the following config:
I get the following error:
Am I missing anything here? |
Hi @byara , thanks for taking the time to look into this.
It's a bit ugly but it's the simplest way to get around prettier's very limiting restrictions. |
Alright. 2 things.
|
No problem. I'll update the README and tests to use that format.
I can remove it, but do we need to worry about backward compatibility? |
Hi @saaryab, I am planning to release this change in the next major version which is 3.x.x to ensure backward compatibiltiy. Could you please change the target branch of this PR to |
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.
Very Nice 👏 and thanks a lot for this PR ❤️ It solves a very critical issue which was due for a long time.
Thanks or applying the changes to README. The issue is that there cannot be 2 decorator plugins at the same time. Therefore, I guess @ayusharma 's suggestion seems to be the best way out to release this under version 3. |
Sounds good - I'll remove the old decorators plugin soon. |
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
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.
🚀
Thanks for the quick review and helpful comments @byara , @ayusharma , and of course for maintaining this useful plugin! |
This PR is a fix for this issue:
#86
Prettier does not allow complex values in the options file, so in order to support experimental Babel plugins with options I added support for JSON strings in the
experimentalBabelParserPluginsList
options array.Please let me know if you have other ideas of how to support options in these plugins.