-
Notifications
You must be signed in to change notification settings - Fork 108
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
refactor(angular-output-target): split up component proxy code generation #302
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM all in all. This is all code I previously approved, and I don't love the idea of double-jeopardy when it comes to code reviews - I left a few comments, but consider them non-blocking/as suggestions
packages/angular-output-target/__tests__/generate-angular-component.spec.ts
Outdated
Show resolved
Hide resolved
const renamedType = `I${componentClassName}${type}`; | ||
return renamedType | ||
.replace(new RegExp(`^${src}$`, 'g'), `${dst}`) | ||
.replace(new RegExp(`([^\\w])${src}([^\\w])`, 'g'), (v, p1, p2) => [p1, dst, p2].join('')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think this can be simplified to:
.replace(new RegExp(`([^\\w])${src}([^\\w])`, 'g'), (v, p1, p2) => [p1, dst, p2].join('')); | |
.replace(new RegExp(`(\W)${src}(\W)`, 'g'), (v, p1, p2) => [p1, dst, p2].join('')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to opt against taking this commit for this PR. If you want to open up another PR to make that isolated change, I can review it 👍 that way in the event of a bug around this code in the future, it will be easier to reason about the timeline of changes.
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build
) was run locally for affected output targetsnpm test
) were run locally and passednpm run prettier
) was run locally and passedPull request type
Please check the type of change your PR introduces:
What is the current behavior?
N/A
What is the new behavior?
Does this introduce a breaking change?
Other information
Split from: #298