Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

Breaking: Only add .implements/.accessibility/.decorators if "truthy" #354

Merged
merged 1 commit into from
Aug 16, 2017

Conversation

JamesHenry
Copy link
Member

@JamesHenry JamesHenry commented Aug 15, 2017

This changes the way implements, accessibility and decorators are applied.

When running babylon AST comparison tests, I noticed that babylon only applies the above properties if they are "truthy" (not exactly the right way to describe it but hopefully it's clear what I mean).

For example, if there is at least one decorator on a node, there will be a decorators array in the AST, otherwise, there will be no decorators property at all.

Currently in typescript-eslint-parser, we usually apply an empty array for the decorators in that case.

Before reviewing and merging this PR, I would first like to confirm that this is indeed the desired behaviour, @Andy-MS?

I am personally pretty ambivalent about which makes more sense and would be keen to hear your thoughts on why you opted from leaving them out of the AST vs empty array/null value.

cc @hzoo @soda0289

@eslintbot
Copy link

LGTM

@JamesHenry JamesHenry force-pushed the implements-accessibility-decorators branch from f69dab5 to 80dee49 Compare August 15, 2017 16:57
@eslintbot
Copy link

Thanks for the pull request, @JamesHenry! I took a look to make sure it's ready for merging and found some changes are needed:

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@ghost
Copy link

ghost commented Aug 15, 2017

This isn't a big issue for me either; I was following the behavior of the flow plugin, which only adds plugin-specific properties (such as .typeAnnotation) if plugin-specific syntax is present. I don't know if this is actually important anywhere in the babel toolchain, but it does make the JSON representation cleaner.

@JamesHenry
Copy link
Member Author

Ok, thanks a lot for the feedback! I can't think of any strong reason not to do that (if downstream tools like prettier or the typescript plugin are assuming the properties exist that will be easy enough to fix) I'll request a review from @soda0289, and he can voice any concerns he may have if any.

Copy link
Member

@soda0289 soda0289 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Might have some issues if plugins expected those properties but I don't think any do.

@JamesHenry JamesHenry merged commit ab4e05e into master Aug 16, 2017
@JamesHenry JamesHenry deleted the implements-accessibility-decorators branch August 16, 2017 14:27
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants