feat(compiler): support directive selectors with attributes containing $

[iRealNirmal]

Added support for $ in attribtue type directive. This feaure is covered as the part of ticket #41244.

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Currently directive selector isn't working if it has $ character in it.

Issue Number: 41244

What is the new behavior?

As per the feature requirement now directive attribute selector is support character $

Does this PR introduce a breaking change?

• Yes
• No

Other information

[petebacondarwin]

Thanks for submitting this PR @iRealNirmal.

Could you please add a unit test similar to https://github.com/angular/angular/blob/master/packages/compiler/test/selector/selector_spec.ts#L116-L127, which demonstrates this fix.

Also, can you modify the commit message slightly. It should not mention the issue number in the commit header, but instead have a Fixes. Close or Resolves at the bottom of the commit body. Something like:

feat(compiler): Support directive selectors with attributes containing `$`

This commit adds support for `$` in when selecting attributes.

Resolves #41244.

[iRealNirmal]

Thanks for submitting this PR @iRealNirmal.

Could you please add a unit test similar to https://github.com/angular/angular/blob/master/packages/compiler/test/selector/selector_spec.ts#L116-L127, which demonstrates this fix.

Also, can you modify the commit message slightly. It should not mention the issue number in the commit header, but instead have a Fixes. Close or Resolves at the bottom of the commit body. Something like:

feat(compiler): Support directive selectors with attributes containing `$`

This commit adds support for `$` in when selecting attributes.

Resolves #41244.

Thanks @petebacondarwin. I have taken care for your both of comment.

But I am not sure why test_aio_preview failed. Will you be able to provide any insight here ?

[petebacondarwin]

Thanks @iRealNirmal - the second commit could have been a "fixup" commit. You can read more about this here: https://github.com/angular/angular/blob/master/docs/FIXUP_COMMITS.md.

Also I made a mistake the first letter of the commit header should not be uppercase.

Could you either change the second commit to a fixup, or just squash both commits into one?

[iRealNirmal]

@petebacondarwin let me know in case anything else needs to be done in this PR.

[crisbeto]

Are we sure that we want to support this? If the selector doesn't match an output name and we end up trying to add it to the DOM, the browser will throw an error. See:

[petebacondarwin]

Are we sure that we want to support this? If the selector doesn't match an output name and we end up trying to add it to the DOM, the browser will throw an error. See:

@crisbeto - That seems like a perfectly reasonable error message for using an attribute incorrectly, no? Or am I missing something? If there is no such output on a directive/component, then the AOT compiler would complain before it got to runtime, no?

[crisbeto]

It's possible that I'm missing some context as well, but wouldn't this also allow for any attribute containing $? E.g. having a <div foo$></div> in a template.

[crisbeto]

I just double-checked it and we already allow <div foo$></div>, even though it results in a runtime error. Please ignore my previous comment.

[petebacondarwin]

I just double-checked it and we already allow <div foo$></div>, even though it results in a runtime error. Please ignore my previous comment.

Thanks for checking

[petebacondarwin]

Please make sure that the following tests pass:

    it('should detect attr names with escaped $', () => {
      let cssSelector = CssSelector.parse('[attrname\\$]')[0];
      expect(cssSelector.attrs).toEqual(['attrname$', '']);
      expect(cssSelector.toString()).toEqual('[attrname\\$]');

      cssSelector = CssSelector.parse('[\\$attrname]')[0];
      expect(cssSelector.attrs).toEqual(['$attrname', '']);
      expect(cssSelector.toString()).toEqual('[\\$attrname]');

      cssSelector = CssSelector.parse('[foo\\$bar]')[0];
      expect(cssSelector.attrs).toEqual(['foo$bar', '']);
      expect(cssSelector.toString()).toEqual('[foo\\$bar]');
    });

    it('should error on attr names with unescaped $', () => {
      expect(() => CssSelector.parse('[attrname$]'))
          .toThrowError(
              'Error in attribute selector "attrname$". Unescaped "$" is not supported. Please escape with "\\$".');
      expect(() => CssSelector.parse('[$attrname]'))
          .toThrowError(
              'Error in attribute selector "$attrname". Unescaped "$" is not supported. Please escape with "\\$".');
      expect(() => CssSelector.parse('[foo$bar]'))
          .toThrowError(
              'Error in attribute selector "foo$bar". Unescaped "$" is not supported. Please escape with "\\$".');
      expect(() => CssSelector.parse('[foo\\$bar$]'))
          .toThrowError(
              'Error in attribute selector "foo\\$bar$". Unescaped "$" is not supported. Please escape with "\\$".');
    });

[petebacondarwin]

Great! I restarted the failing jobs - they are most likely flakes.
Just one update to the tests are needed.

[petebacondarwin]

Awesome work. Thanks @iRealNirmal

[AndrewKushnir]

LGTM, thanks for this PR 👍

I've added a comment to expand internal docs (add JSDoc for newly added functions), so that there is context to simplify future maintenance of that code.

[AndrewKushnir]

Presubmit.

[petebacondarwin]

A couple of tiny fixes to the docs.

[petebacondarwin]

LGTM!

The requested changes have been made.

[AndrewKushnir]

Presubmit #2.

[angular-automatic-lock-bot]

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.}