Skip to content

Add missing methods for FormData type #219

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
Mar 29, 2017
Merged

Add missing methods for FormData type #219

merged 5 commits into from
Mar 29, 2017

Conversation

NColey
Copy link
Contributor

@NColey NColey commented Mar 26, 2017

Fixes microsoft/TypeScript#14813 by updating the existing append method to better match the Web APIs and adding missing methods. Please let me know if there is anything I should add or change, thanks!

@msftclas
Copy link

@NColey,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

get(name: any): any;
getAll(name: any): any;
has(name: any): any;
set(name: any, value: any, fileName?: string): void;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the spec: https://xhr.spec.whatwg.org/#formdata the definition should look like:

type FormDataEntryValue = File or string;

interface FormData {
  append(name: string, value: string): void;
  append(name:string, blobValue:Blob, filename?:string):void;
  delete(name: string):void;
  get(name:string):  FormDataEntryValue | null;
  getAll(name: string): FormDataEntryValue[];
  has(name: string): boolean;
  set(name:string, value:string): void;
  set(name: string, blobValue:Blob, filename?: string): void;
};

Choose a reason for hiding this comment

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

You can add arbitrary Blob, but you can only get File back? interesting

@NColey
Copy link
Contributor Author

NColey commented Mar 29, 2017

My apologies @mhegazy , I had been referencing the definitions here: https://developer.mozilla.org/en-US/docs/Web/API/FormData. I just updated the PR to match the appropriate definitions from the spec, thank you for sharing the link with me!

@@ -3796,7 +3796,12 @@ declare var FocusNavigationEvent: {
}

interface FormData {
append(name: any, value: any, blobName?: string): void;
append(name: string, value: any, fileName?: string): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Still have a few anys left:

type FormDataEntryValue = File | string;

interface FormData {
  append(name: string, value: string): void;
  append(name:string, blobValue:Blob, filename?:string):void;
  delete(name: string):void;
  get(name:string):  FormDataEntryValue | null;
  getAll(name: string): FormDataEntryValue[];
  has(name: string): boolean;
  set(name:string, value:string): void;
  set(name: string, blobValue:Blob, filename?: string): void;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, I have a quick question before I update my PR, if I can't find a FormDataEntryValue interface in the generated file does that make it ok to add it as a type anyway? Or should that be added first?

Choose a reason for hiding this comment

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

That is correct, it's just a type so it should not be in the generated code, but it should be available to users to use as a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much, that makes sense! I just updated the PR with the appropriate types added in.

Choose a reason for hiding this comment

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

Ah, looking in the full diff I understand better what you meant.

It seems you need to add FormDataEntryValue to the json file as a typedef (search "typedef" in it to find examples)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kovensky Is there another area beyond just as a typedef where it might need to be added? I just updated my PR with the typedef for FormDataEntryValue but the test is failing with the following error Error: The baseline file verifyDefaultLIb_dom.errors.txt has changed. which is both a file that I don't seem to have and also one I suspect I shouldn't touch. I can't seem to figure out what in my changes would have prompted that but I'm guessing there is another area of the code where I would need to add FormDataEntryValue?

@@ -3796,7 +3796,12 @@ declare var FocusNavigationEvent: {
}

interface FormData {
append(name: any, value: any, blobName?: string): void;
append(name: string, value: string | blob, fileName?: string): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mean Blob, my guess is this is what is causing the test failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right, thanks so much for catching that. I just fixed it and it seems to be passing now. Thank you!

@mhegazy mhegazy merged commit 56a53e1 into microsoft:master Mar 29, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Mar 29, 2017

thanks!

@NColey
Copy link
Contributor Author

NColey commented Mar 29, 2017

Thank you!

@NColey NColey deleted the form-data-methods branch March 29, 2017 19:08
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.

4 participants