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(ivy): wrap "inputs" and "outputs" keys if they contain unsafe chars #28919

Closed

Conversation

AndrewKushnir
Copy link
Contributor

Prior to this change, keys in "inputs" and "outputs" objects generated by compiler were not checked against unsafe characters. As a result, in some cases the generated code was throwing JS error. Now we check whether a given key contains any unsafe chars and wrap it in quotes if needed.

This PR resolves ticket FW-1096.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Feb 22, 2019
@AndrewKushnir AndrewKushnir requested a review from a team as a code owner February 22, 2019 05:36
@ngbot ngbot bot added this to the needsTriage milestone Feb 22, 2019
@alfaproject
Copy link
Contributor

Why not just quote all of them and let the minifier do its job?

Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Help me understand the impact of this more.

The unquoted keys are unquoted so the optimizer will rename them (just as it renames the actual input field on the class). What impact does quoting here have on this renaming? Is the assertion that any property which has special characters will not be renamed by the optimizer?

Should we consider forbidding input names that are not valid JS identifiers?

* quotes. Note: we do not wrap all keys into quotes, as it may have impact on minification and may
* bot work in some cases when object keys are mangled by minifier.
*/
const UNSAFE_OBJECT_KEY_NAME_REGEXP = /-/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why - only? I can have whatever text value as property name like inputs: ['...foo:foo', ',,,bar:bar'].

Copy link
Member

Choose a reason for hiding this comment

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

Currently we're having problems with - in input names.

This is a temporary solution while we discuss what to do about this particular issue. Options include:

  1. adding some heuristic to decide whether a property will likely be accessed via minified (this.bar) or unminified (this.other-bar) references, and quoting the property mapping for Ivy accordingly.

  2. prohibiting the use of input names that aren't valid JS identifiers in the first place (which would be a breaking change).

@trotyl
Copy link
Contributor

trotyl commented Mar 2, 2019

Should we consider forbidding input names that are not valid JS identifiers?

@alxhub A class property can indeed be whatever* string or symbol:

class Foo {
  'a$b*c' = 42
  ['d@e!f'] = 84
  [mySymbol] = 21
}

*: Here only means any character, not special names like constructor.

As a top-goal of Ivy is to reduce compiler restriction and support any JavaScript usage when possible, I don't think this is a viable approach.

@trotyl
Copy link
Contributor

trotyl commented Mar 2, 2019

@alxhub Please also note that a property named in valid identifier may also be quoted:

class Foo {
    prop0 = 42;
    'prop1' = 42;
    ['prop2'] = 42;
}

In above case the quotes will be preserved in compilation result and thus not being minified by closure compiler.

The true fix should be dropping inputs/outputs in Component metadata (as originally designed for ES5 DSL which has been removed) and only supports @Input()/@Output() property decorators. Otherwise there's no way to understand whether or not the property is quoted (possibly defined in a non-Angular base class).

@AndrewKushnir AndrewKushnir added action: discuss and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 2, 2019
…racters

Prior to this change, keys in "inputs" and "outputs" objects generated by compiler were not checked against unsafe characters. As a result, in some cases the generated code was throwing JS error. Now we check whether a given key contains any unsafe chars and wrap it in quotes if needed.
@AndrewKushnir
Copy link
Contributor Author

Thanks @trotyl and @alxhub for the input. I've added a TODO to indicate that this solution is a temporary one to avoid confusion. Thank you.

@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Mar 4, 2019

Presubmit

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Mar 4, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants