Support passing babel plugin configuration in JSON #88

saaryab · 2021-10-13T16:10:12Z

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.

saaryab · 2021-10-13T16:50:56Z

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?
Thanks!

byara · 2021-10-14T07:51:29Z

Thank you @saaryab for the PR. We are looking into the PR and will release a new version very soon.

avshalomt · 2021-10-14T08:45:42Z

src/preprocessor.ts


    const importNodes: ImportDeclaration[] = [];

    const parserOptions: ParserOptions = {
        sourceType: 'module',
-        plugins: [...plugins, ...experimentalBabelParserPluginsList],
+        plugins: [...plugins, ...experimentalParserPlugins],


Can you keep the word babel in the name so it matches the config variable name?

avshalomt · 2021-10-14T08:52:22Z

src/utils/get-experimental-parser-plugins.ts

+    // 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


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:

Suggested change

pluginName => pluginName.startsWith("[") ? JSON.parse(pluginName) : pluginName

pluginName => tryParseJson(pluginName) ?? pluginName,

Prettier requires plugins like this one to declare the options fields they add in options in index.ts. The types available are only 'int' | 'boolean' | 'choice' | 'path' (source), and an additional array flag. Prettier then does it's own run-time type validation on the options and prints this error if I send an array in the experimentalBabelParserPluginsList:

[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

Bahh that sucks :/ So your JSON solution sounds good

Cool

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.

avshalomt · 2021-10-14T09:27:59Z

src/utils/get-experimental-parser-plugins.ts

+    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("[");


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.

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

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.

byara · 2021-10-15T09:53:52Z

Hey @saaryab, I took a look at the PR and tried to test it on a sample project and by running prettier
./node_modules/.bin/prettier --config .prettierrc --write '**/*.{ts,tsx,js}'

with the following config:

{
    "printWidth": 80,
    "tabWidth": 4,
    "trailingComma": "all",
    "singleQuote": true,
    "semi": true,
    "importOrder": ["^@server/(.*)$", "^@core/(.*)$", "^@ui/(.*)$", "^[./]"],
    "importOrderSeparation": true,
    "experimentalBabelParserPluginsList": [["decorators", { "decoratorsBeforeExport": true }]]
}

I get the following error:

[error] Invalid experimentalBabelParserPluginsList value. Expected an array of a string, but received [["decorators", { decoratorsBeforeExport: true }]].

Am I missing anything here?

saaryab · 2021-10-15T10:08:54Z

Hi @byara , thanks for taking the time to look into this.
The error you are getting is the reason for the PR - prettier does run time validation and only accepts primitive types in the options array.
To get around that you can add single quotes around the inner array so it's a JSON string and not an array.

'["decorators", { "decoratorsBeforeExport": true }]'

It's a bit ugly but it's the simplest way to get around prettier's very limiting restrictions.

byara · 2021-10-15T12:58:15Z

Alright. 2 things.

I took a deeper look and gotta admit I like the following structure better, which at least we are not going to have error messages in any IDEs:

"experimentalBabelParserPluginsList": ["[\"decorators\", { \"decoratorsBeforeExport\": true }]", "classProperties"]

Now that we are adding this, we need to remove the legacy decorator that we are adding automatically in getParserPlugins in src/utils/get-parser-plugins.ts

saaryab · 2021-10-15T13:18:24Z

I took a deeper look and gotta admit I like the following structure better, which at least we are not going to have error messages in any IDEs:
"experimentalBabelParserPluginsList": ["[\"decorators\", { \"decoratorsBeforeExport\": true }]", "classProperties"]

No problem. I'll update the README and tests to use that format.

Now that we are adding this, we need to remove the legacy decorator that we are adding automatically in getParserPlugins in src/utils/get-parser-plugins.ts

I can remove it, but do we need to worry about backward compatibility?
Maybe someone is relying on it and they will need to update their prettierrc file after upgrading this package.

ayusharma · 2021-10-15T14:07:31Z

I can remove it, but do we need to worry about backward compatibility?
Maybe someone is relying on it and they will need to update their prettierrc file after upgrading this package.

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 v3.x. ( Edited: I change it to 3.x)

ayusharma

Very Nice 👏 and thanks a lot for this PR ❤️ It solves a very critical issue which was due for a long time.

byara · 2021-10-15T14:16:04Z

I took a deeper look and gotta admit I like the following structure better, which at least we are not going to have error messages in any IDEs:
"experimentalBabelParserPluginsList": ["[\"decorators\", { \"decoratorsBeforeExport\": true }]", "classProperties"]
No problem. I'll update the README and tests to use that format.

Now that we are adding this, we need to remove the legacy decorator that we are adding automatically in getParserPlugins in src/utils/get-parser-plugins.ts

I can remove it, but do we need to worry about backward compatibility? Maybe someone is relying on it and they will need to update their prettierrc file after upgrading this package.

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.

saaryab · 2021-10-15T14:18:38Z

Sounds good - I'll remove the old decorators plugin soon.

src/utils/get-parser-plugins.ts

byara

LGTM

ayusharma

🚀

saaryab · 2021-10-15T14:59:11Z

Thanks for the quick review and helpful comments @byara , @ayusharma , and of course for maintaining this useful plugin!

Support passing babel plugin configuration in JSON

d5c3443

saaryab mentioned this pull request Oct 13, 2021

Support babel plugin options in experimentalBabelParserPluginsList #86

Closed

saaryab added 3 commits October 13, 2021 19:24

Clean up extra spaces

fbab9be

Fix comment

29da002

Syntax conventions

8d75eb8

byara requested review from ayusharma and byara October 14, 2021 07:48

byara added the enhancement New feature or request label Oct 14, 2021

This was linked to issues Oct 14, 2021

Support babel plugin options in experimentalBabelParserPluginsList #86

Closed

Decorators problem for Angular project. #12

Closed

avshalomt suggested changes Oct 14, 2021

View reviewed changes

saaryab added 3 commits October 14, 2021 12:20

Clearer naming

fc39ce2

Clearer parsing of plugin options as JSON

da7e32c

conventions

a8a3d23

avshalomt reviewed Oct 14, 2021

View reviewed changes

saaryab added 4 commits October 14, 2021 12:28

remove older comment

17d6321

removeunused import

37c8374

add types

04161fd

add documentation

c0377f7

Merge branch 'master' into master

8f6ff7c

saaryab added 2 commits October 15, 2021 16:36

Use escaped double quote strings

5c4bcfb

Merge branch 'master' of github.com:saaryab/prettier-plugin-sort-imports

25019bf

ayusharma approved these changes Oct 15, 2021

View reviewed changes

ayusharma changed the base branch from master to v3.x October 15, 2021 14:12

byara approved these changes Oct 15, 2021

View reviewed changes

Merge branch 'v3.x' into master

16adea7

Remove decorators-legacy babel plugin

accb8b9

saaryab commented Oct 15, 2021

View reviewed changes

src/utils/get-parser-plugins.ts Outdated Show resolved Hide resolved

byara mentioned this pull request Oct 15, 2021

Parser fails to decode object castings #81

Closed

byara linked an issue Oct 15, 2021 that may be closed by this pull request

Parser fails to decode object castings #81

Closed

Remove classProperties babel plugin from default TS plugins

3ab9e39

byara requested a review from ayusharma October 15, 2021 14:52

byara approved these changes Oct 15, 2021

View reviewed changes

ayusharma approved these changes Oct 15, 2021

View reviewed changes

ayusharma merged commit 486cfa0 into trivago:v3.x Oct 15, 2021

ayusharma added this to In progress in Version 3: New features on Babel compiler via automation Oct 18, 2021

byara moved this from In progress to Done in Version 3: New features on Babel compiler Oct 19, 2021

blutorange mentioned this pull request Nov 17, 2021

Running with mobx decorator got error #109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support passing babel plugin configuration in JSON #88

Support passing babel plugin configuration in JSON #88

saaryab commented Oct 13, 2021

saaryab commented Oct 13, 2021

byara commented Oct 14, 2021

avshalomt Oct 14, 2021

saaryab Oct 14, 2021

avshalomt Oct 14, 2021

saaryab Oct 14, 2021

avshalomt Oct 14, 2021

saaryab Oct 14, 2021 •

edited

avshalomt Oct 14, 2021

avshalomt Oct 14, 2021

saaryab Oct 14, 2021

byara commented Oct 15, 2021

saaryab commented Oct 15, 2021 •

edited

byara commented Oct 15, 2021

saaryab commented Oct 15, 2021

ayusharma commented Oct 15, 2021 •

edited

ayusharma left a comment

byara commented Oct 15, 2021

saaryab commented Oct 15, 2021

byara left a comment

ayusharma left a comment

saaryab commented Oct 15, 2021

	pluginName => pluginName.startsWith("[") ? JSON.parse(pluginName) : pluginName
	pluginName => tryParseJson(pluginName) ?? pluginName,

Support passing babel plugin configuration in JSON #88

Support passing babel plugin configuration in JSON #88

Conversation

saaryab commented Oct 13, 2021

saaryab commented Oct 13, 2021

byara commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saaryab Oct 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byara commented Oct 15, 2021

saaryab commented Oct 15, 2021 • edited

byara commented Oct 15, 2021

saaryab commented Oct 15, 2021

ayusharma commented Oct 15, 2021 • edited

ayusharma left a comment

Choose a reason for hiding this comment

byara commented Oct 15, 2021

saaryab commented Oct 15, 2021

byara left a comment

Choose a reason for hiding this comment

ayusharma left a comment

Choose a reason for hiding this comment

saaryab commented Oct 15, 2021

saaryab Oct 14, 2021 •

edited

saaryab commented Oct 15, 2021 •

edited

ayusharma commented Oct 15, 2021 •

edited