Skip to content

Allow actual RegExp objects in the regex field of a textInput #1247

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

Merged
merged 5 commits into from
Oct 6, 2022

Conversation

Berlioz
Copy link
Contributor

@Berlioz Berlioz commented Sep 30, 2022

I'm not....proud of this hack, but I was writing up some docs for params and frankly I couldn't get the slash escapes right in my head when typing them out, which means it's really not reasonable to expect users to get them right either. Much better to just let them use the same JS regexps that they're used to using, and can copy-paste test.

Is this going to require APi review? The actual wire format hasn't changed.

@@ -102,6 +102,14 @@ export interface ManifestStack {
* @internal
*/
export function stackToWire(stack: ManifestStack): Record<string, unknown> {
if (stack.params) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In retrospect, I think there's a good argument to be made that this ugly special case should probably live in the generic toSpec() implementation in params/types.ts?

Copy link
Contributor

Choose a reason for hiding this comment

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

nice "hack" with super great usability upgrade. Do you want to do that here or on a separate PR.

@@ -102,6 +102,14 @@ export interface ManifestStack {
* @internal
*/
export function stackToWire(stack: ManifestStack): Record<string, unknown> {
if (stack.params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice "hack" with super great usability upgrade. Do you want to do that here or on a separate PR.

@taeold taeold added this to the v4 milestone Oct 6, 2022
@Berlioz
Copy link
Contributor Author

Berlioz commented Oct 6, 2022

  1. it turns out that we do not need API review to do it like this for the slightly hilarious reason that i misread the design doc and we were always supposed to do it like this
  2. the active ingredient of this patch has indeed been moved into toSpec() as we discussed

@Berlioz Berlioz merged commit 9a7b280 into launch.next Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants