change EventGroup.declare to string literal #14085
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
because typescript supports class declarations such as `class X {
public declare x: number
}` , using declare has the potential to cause problems for ts parsers supporting classes declaring types. Specifically, this causes an issue with the latest version sucrase. Although the responsibility falls on the parser, it seems pertinent to just avoid this. Changing to ["declare"] is an immediate fix, while renaming it to EventGroup.declareEvent is probably a better long term change.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 54513ce:
|
ecraig12345
reviewed
Jul 17, 2020
packages/utilities/src/EventGroup.ts
Outdated
Comment on lines
310
to
312
| //see https://github.com/alangpierce/sucrase/issues/545 | ||
| /** Declare an event as being supported by this instance of EventGroup. */ | ||
| public declare(event: string | string[]): void { | ||
| public ['declare'](event: string | string[]): void { |
There was a problem hiding this comment.
I'd recommend adding a short inline explanation for the use of quotes, in addition to the link. Otherwise it's not clear what the link is referring to.
|
|
|
Hello @ecraig12345! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ecraig12345
approved these changes
Sep 4, 2020
|
@jeremy-coleman Looks like you'll need to sign the CLA before this can merge--you should have gotten a comment about that from the CLA bot when you first opened the PR, but I think it was broken for awhile. |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Potential regressions comparing to master
Perf comparison
Perf tests with no regressions
|
|
This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
because typescript supports class declarations such as
class X { public declare x: number }, using declare has the potential to cause problems for ts parsers supporting classes declaring types. Specifically, this causes an issue with the latest version sucrase. Although the responsibility falls on the parser, it seems pertinent to just avoid this. Changing to ["declare"] is an immediate fix, while renaming it to EventGroup.declareEvent is probably a better long term change. See alangpierce/sucrase#545 for reference.Pull request checklist
$ yarn changeDescription of changes
(give an overview)
Focus areas to test
(optional)