fix(@schematics/angular): allow dash in selector before a number by sumitparakh · Pull Request #25253 · angular/angular-cli

sumitparakh · 2023-05-20T13:55:37Z

PR Checklist

Please check to confirm your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular-cli/blob/main/CONTRIBUTING.md#-commit-message-guidelines
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?

What is the current behavior?

Does not allow dash in selector before a number. Which means, following command would throw error: -
ng g c TodoItemTask-123

Issue Number: #25164

What is the new behavior?

Command like this will work: -
ng g c TodoItemTask-123

Does this PR introduce a breaking change?

Yes
No

Other information

google-cla · 2023-05-20T13:55:41Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

alan-agius4

Thanks for your contribution.

Can you please replace the E2E with a Unit test, similar to:

angular-cli/packages/schematics/angular/application/index_spec.ts

Lines 512 to 517 in e037dc9

    
           it('should support creating applications with `_` and `.` in name', async () => { 
        
             const options = { ...defaultOptions, name: 'foo.bar_buz' }; 
        
             const tree = await schematicRunner.runSchematic('application', options, workspaceTree); 
        
             expect(tree.exists('/projects/foo.bar_buz/tsconfig.app.json')).toBeTrue(); 
        
           });

Also, kindly update the commit message to

fix(@schematics/angular): allow dash in selector before a number

Closes #25164

sumitparakh · 2023-05-24T05:31:38Z

Hi @alan-agius4 , I have made the suggested changes but the setup step failed with 500 internal server error. Does not seem to be related to my code.

alan-agius4 · 2023-05-24T15:51:32Z

Can you please clean you the commit message description and remove

fix(@angular/cli): fix regex

fix(@angular/cli): updated test case

fix(@angular/cli): fix "too big tests" error in ci

added a new file to test dash with number case

if you do want to add a description it should be before the closes and it should describe the change. Ex:


fix(@schematics/angular): allow dash in selector before a number

This commit updates the validator regexp to allow a dash before a number.

Closes #25164

sumitparakh · 2023-05-24T16:48:09Z

Can you please clean you the commit message description and remove
fix(@angular/cli): fix regex

fix(@angular/cli): updated test case

fix(@angular/cli): fix "too big tests" error in ci

added a new file to test dash with number case
if you do want to add a description it should be before the closes and it should describe the change. Ex:
fix(@schematics/angular): allow dash in selector before a number

This commit updates the validator regexp to allow a dash before a number.

Closes #25164

Done @alan-agius4. :)

alan-agius4 · 2023-05-30T08:42:25Z

packages/schematics/angular/utility/validation.ts

 // Must start with a letter, and must contain only alphanumeric characters or dashes.
 // When adding a dash the segment after the dash must also start with a letter.
-export const htmlSelectorRe = /^[a-zA-Z][.0-9a-zA-Z]*(:?-[a-zA-Z][.0-9a-zA-Z]*)*$/;
+export const htmlSelectorRe = /^[a-zA-Z][.0-9a-zA-Z]*(:?-[a-zA-Z][.0-9a-zA-Z]*(:?-[0-9]+)*)*$/;


In some cases this regexp will fail to match

Example:

one-1

Maybe we can use the same logic as https://github.com/angular/angular-cli/blob/b66d94e5949102b2235b89cb75866973986ea30a/packages/angular_devkit/schematics/src/formats/html-selector.ts#L17C1-L44

//cc @clydin do you recall why we support upper characters as based on the spec it seems that these are not valid. https://html.spec.whatwg.org/multipage/custom-elements.html#valid-custom-element-name

Hi @alan-agius4 ,

It works when we create component with a general command like this: ng g c one-1. In this case, the final selector becomes: app-one-1 which is valid for this regexp. Assuming that the default prefix app is being used.

However, it does not work with following command: ng g c one-1 --selector one-1 because it tries to override the default naming convention of selector in which prefix is added initially.

I will cover this scenario in my next commit and will update.

@alan-agius4 Found another problem. If we allow one-1 like selectors then it will also verify app-1-one as valid and in that case, the component class name becomes like this: -

export class 1OneComponent { }

This is invalid class name. Isn't it?

So, I might have to add some conditions that regexp doesn't allow 1-one if the component is being created with a prefix which will ultimately become app-1-one.

sumitparakh · 2023-06-03T11:04:04Z

packages/schematics/angular/utility/validation.ts

 // When adding a dash the segment after the dash must also start with a letter.
-export const htmlSelectorRe = /^[a-zA-Z][.0-9a-zA-Z]*(:?-[a-zA-Z][.0-9a-zA-Z]*)*$/;
+export const htmlSelectorRe =
+  /^[a-zA-Z][.0-9a-zA-Z]*((:?-[0-9]+)*|(:?-[a-zA-Z][.0-9a-zA-Z]*(:?-[0-9]+)*)*)$/;


Regexp used here doesn't validate html selector correctly. For eg., it considers foo- as a valid selector which is wrong, right? And it also allows underscore.

I have modified regexp in validation.ts file itself to cover selectors like one-1 but if we still want to use the logic at html-selector.ts then we might have to make some changes in that logic.

This commit updates the validator regexp to allow a dash before a number. Closes angular#25164

sumitparakh · 2023-06-09T20:37:32Z

Hi @alan-agius4, I have made some changes. Could you please review this?

alan-agius4

LGTM

angular-automatic-lock-bot · 2023-12-02T00:08:29Z

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

sumitparakh force-pushed the sparakh-bugfix-25164 branch from eb464f1 to fdebed0 Compare May 20, 2023 14:00

sumitparakh marked this pull request as draft May 21, 2023 11:12

sumitparakh force-pushed the sparakh-bugfix-25164 branch 2 times, most recently from 80abd64 to 5fe6cff Compare May 22, 2023 05:15

sumitparakh marked this pull request as ready for review May 22, 2023 14:44

alan-agius4 requested changes May 23, 2023

View reviewed changes

alan-agius4 added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels May 23, 2023

sumitparakh force-pushed the sparakh-bugfix-25164 branch from 5fe6cff to b3d50ef Compare May 24, 2023 05:28

sumitparakh changed the title ~~fix(@angular/cli): allow dash in selector before a number~~ fix(@schematics/angular): allow dash in selector before a number May 24, 2023

sumitparakh requested a review from alan-agius4 May 24, 2023 13:33

sumitparakh force-pushed the sparakh-bugfix-25164 branch from b53f320 to 4dc8705 Compare May 24, 2023 16:47

alan-agius4 reviewed May 30, 2023

View reviewed changes

sumitparakh force-pushed the sparakh-bugfix-25164 branch from 4dc8705 to d6c4610 Compare May 30, 2023 19:45

sumitparakh marked this pull request as draft May 30, 2023 19:47

sumitparakh force-pushed the sparakh-bugfix-25164 branch from d6c4610 to 55392c4 Compare June 3, 2023 11:01

sumitparakh commented Jun 3, 2023

View reviewed changes

sumitparakh force-pushed the sparakh-bugfix-25164 branch from 55392c4 to a7e1943 Compare June 3, 2023 11:07

fix(@schematics/angular): allow dash in selector before a number

6c4c56e

This commit updates the validator regexp to allow a dash before a number. Closes angular#25164

sumitparakh force-pushed the sparakh-bugfix-25164 branch from a7e1943 to 6c4c56e Compare June 3, 2023 11:08

sumitparakh marked this pull request as ready for review June 3, 2023 11:22

sumitparakh requested a review from alan-agius4 June 3, 2023 11:22

alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Sep 11, 2023

alan-agius4 removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 26, 2023

alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Oct 26, 2023

alan-agius4 approved these changes Oct 26, 2023

View reviewed changes

alan-agius4 merged commit c5827d4 into angular:main Oct 26, 2023

sumitparakh deleted the sparakh-bugfix-25164 branch November 1, 2023 05:57

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(@schematics/angular): allow dash in selector before a number#25253

fix(@schematics/angular): allow dash in selector before a number#25253
alan-agius4 merged 1 commit intoangular:mainfrom
sumitparakh:sparakh-bugfix-25164

sumitparakh commented May 20, 2023 •

edited

Loading

Uh oh!

google-cla bot commented May 20, 2023

Uh oh!

alan-agius4 left a comment

Uh oh!

sumitparakh commented May 24, 2023

Uh oh!

alan-agius4 commented May 24, 2023

Uh oh!

sumitparakh commented May 24, 2023

Uh oh!

alan-agius4 May 30, 2023

Uh oh!

sumitparakh May 30, 2023

Uh oh!

sumitparakh May 30, 2023 •

edited

Loading

Uh oh!

sumitparakh Jun 3, 2023

Uh oh!

sumitparakh commented Jun 9, 2023

Uh oh!

alan-agius4 left a comment

Uh oh!

angular-automatic-lock-bot bot commented Dec 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	it('should support creating applications with `_` and `.` in name', async () => {
	const options = { ...defaultOptions, name: 'foo.bar_buz' };
	const tree = await schematicRunner.runSchematic('application', options, workspaceTree);

	expect(tree.exists('/projects/foo.bar_buz/tsconfig.app.json')).toBeTrue();
	});

Conversation

sumitparakh commented May 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

google-cla bot commented May 20, 2023

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

sumitparakh commented May 24, 2023

Uh oh!

alan-agius4 commented May 24, 2023

Uh oh!

sumitparakh commented May 24, 2023

Uh oh!

alan-agius4 May 30, 2023

Choose a reason for hiding this comment

Uh oh!

sumitparakh May 30, 2023

Choose a reason for hiding this comment

Uh oh!

sumitparakh May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumitparakh Jun 3, 2023

Choose a reason for hiding this comment

Uh oh!

sumitparakh commented Jun 9, 2023

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Dec 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sumitparakh commented May 20, 2023 •

edited

Loading

sumitparakh May 30, 2023 •

edited

Loading