Skip to content
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

fix: make @api decorator spec compliant #572

Merged
merged 2 commits into from
Aug 12, 2018
Merged

Conversation

diervo
Copy link
Contributor

@diervo diervo commented Aug 12, 2018

Details

With the latests changes on the spec, a decorator on a getter/setter applies to both. This changes update the descriptor config to ensure that we push the right bitmask values.

In a nutshell:

If @api is declared in a getter/setter and its counterpart exist, its equivalent to have @api in both the getter and the setter

Does this PR introduce a breaking change?

  • Yes
  • No

@diervo diervo requested review from pmdartus and apapko August 12, 2018 02:05
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 42e7e0a | Target commit: 8289b5a

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 42e7e0a | Target commit: 074a1e8

@diervo diervo merged commit deeb6bb into master Aug 12, 2018
@diervo diervo deleted the dval/apiDecoratorSpecFix branch August 12, 2018 03:05
@caridy
Copy link
Contributor

caridy commented Aug 12, 2018

@pmdartus @apapko did you guys get a chance to review this?

@diervo
Copy link
Contributor Author

diervo commented Aug 13, 2018

I did not gave them the change, but I run all tests in all repos and in core, and manually verify all of it ;)

// We need to add the proper bitmask for the sibling getter/setter if exists
const siblingPair = getSiblingGetSetPair(property, propertyName, type);
if (siblingPair) {
acc[propertyName].config |= getPropertyBitmask(siblingPair.type);
Copy link
Member

Choose a reason for hiding this comment

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

What about completely removing the bitmask? We are not using it's capability, all the public properties should have a getter and a setter.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean that we don't use it? We do use it in decorators/api.ts:

    const config = (!isUndefined(meta) && hasOwnProperty.call(target, 'publicProps') && hasOwnProperty.call(meta, propName)) ? meta[propName].config : 0;

Eventually we will be able to remove it, but for now, we can't.

@diervo
Copy link
Contributor Author

diervo commented Aug 13, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants