Skip to content
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

BREAKING CHANGE(core): Introduce response.data with support for form-urlencoded and custom parsing #211

Merged
merged 3 commits into from
May 14, 2020

Conversation

FokkeZB
Copy link
Contributor

@FokkeZB FokkeZB commented May 2, 2020

Definitions:

Commits

I've split the work up in 3 commits:

Changes

  • prepareContentResponse now parses .content to .data if its either application/x-www-form-urlencoded or can be parsed as JSON.
  • prepareContentResponse still sets .json but it should be considered deprecated.
  • executeHttpRequest now uses .data instead of (again) parsing .content as application/x-www-form-urlencoded or JSON.
  • executeHttpRequest now throws a helpful error instead of SyntaxError if .data is not set and thus not application/x-www-form-urlencoded, JSON or set in afterResponse.

See the first commit.

Benefits

  • Code-mode no longer need to parse application/x-www-form-urlencoded, just like form-mode requests.
  • Developers don't need to JSON.stringify() their custom-parsed .content back to the same property for form-mode to work.
  • The type-agnostic .data property allows us to later add support for types like XML.

See the below Examples.

Examples

These examples all use afterResponse. The code-mode ones could also handle this within the function of course, but this makes it easier to compare between code and form-mode.

Form-mode request with form response

Already and still works out of the box. It just now relies on response.data set by prepareResponse instead of parsing response.content just before returning it.

This means it may be a breaking change for CLI apps that were doing custom parsing in afterResponse as the Form-mode request with XML response example.

Code-mode request with form response

Before

const querystring = require('querystring');

const afterResponse = (response) => {
  if (response.headers.get('content-type') === 'application/x-www-form-urlencoded') {
    response.json = querystring.parse(response.content);
  }
}

After

z.request('https://example.com/').then(response => response.data);

Code-mode request with JSON response

You can still use response.json but we encourage switching to response.data.

Before

z.request('https://example.com/').then(response => response.json);

After

z.request('https://example.com/').then(response => response.data);

Form-mode request with XML response

Note that both before and after currently are only possible in CLI apps, but once https://github.com/zapier/zapier/pull/39899 makes the Middleware tab public for all, this can also be used in visual builder apps.

Before

As we defaulted to parse response.content as JSON:

const parser = require('fast-xml-parser');

const afterResponse = (response) => {
  if ('xml' in response.headers.get('content-type')) {
    const parsed = parser(response.content);
    response.content = JSON.stringify(parsed);
  }
}

After

As we now use response.data:

const parser = require('fast-xml-parser');

const afterResponse = (response) => {
  if ('xml' in response.headers.get('content-type')) {
    response.data = parser(response.content);
  }
}

Code-mode request with XML response

Before

const parser = require('fast-xml-parser');

const afterResponse = (response) => {
  if ('xml' in response.headers.get('content-type')) {
    response.json = parser(response.content);
  }
}

After:

const parser = require('fast-xml-parser');

const afterResponse = (response) => {
  if ('xml' in response.headers.get('content-type')) {
    response.data = parser(response.content);
  }
}

@FokkeZB FokkeZB self-assigned this May 2, 2020
@FokkeZB FokkeZB changed the title WIP: Introduce response.data BREAKING CHANGE(core): Introduce response.data with support for form-urlencoded and custom parsing May 6, 2020
@FokkeZB FokkeZB force-pushed the responseData branch 2 times, most recently from 6ca1e9d to 36c160c Compare May 6, 2020 14:06
@FokkeZB
Copy link
Contributor Author

FokkeZB commented May 6, 2020

@szchenghuang a heads-up that I'd like to include this in v10 as well. It's the last piece of the puzzle that makes afterResponse work well with shorthand/form-mode request. I'll have it ready for review this week.

@xavdid
Copy link
Contributor

xavdid commented May 13, 2020

@FokkeZB we're getting v10 ready to go out the door - do you have a timeline on this?

@FokkeZB
Copy link
Contributor Author

FokkeZB commented May 13, 2020

@xavdid I've got it almost finished. I was prioritizing https://github.com/zapier/zapier/pull/39898 over this one, but I now realize that we won't need to switch visual builder apps immediately, so this should come first as we don't want to have another breaking change soon after.

I'll work on this this afternoon and also prioritize writing docs on both with lots of examples.

@FokkeZB FokkeZB force-pushed the responseData branch 2 times, most recently from 2984e7b to 16dcf3b Compare May 13, 2020 13:26
@FokkeZB FokkeZB marked this pull request as ready for review May 13, 2020 13:26
@FokkeZB FokkeZB added core documentation Improvements or additions to documentation labels May 13, 2020
@FokkeZB FokkeZB force-pushed the responseData branch 2 times, most recently from 553bb70 to 439a596 Compare May 13, 2020 13:51
@eliangcs
Copy link
Member

@FokkeZB quick question: looks like response.json will continue to work, so is it still a breaking change?

@FokkeZB
Copy link
Contributor Author

FokkeZB commented May 13, 2020

@eliangcs it would be breaking for CLI apps that currently change response.content in their afterResponse middleware, with the expectation for form-mode requests to pick up on that. They'll have to replace response.content = JSON.stringify(parsedOrTransformed) with response.data = parsedOrTransformed.

Copy link
Contributor Author

@FokkeZB FokkeZB left a comment

Choose a reason for hiding this comment

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

self-review of changes

Comment on lines +16 to +21
if (response.data === undefined) {
throw new Error(
'Response needs to be JSON, form-urlencoded or parsed in middleware.'
);
}
return createJSONtool().parse(resp.content);
return response.data;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows afterResponse to do any custom parsing and override response.data.

Copy link
Contributor Author

@FokkeZB FokkeZB left a comment

Choose a reason for hiding this comment

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

self-review added tests

@@ -616,4 +617,66 @@ describe('create-app', () => {
);
});
});

describe('response.content parsing', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds test to ensure that form-mode/shorthand request still work with both JSON and form-urlencoded. We didn't have any tests for it AFAIK :(

@@ -529,3 +529,151 @@ describe('http logResponse after middleware', () => {
});
});
});

describe('http prepareResponse', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds tests for prepareResponse for which AFAIK we didn't have any.

Copy link
Contributor Author

@FokkeZB FokkeZB left a comment

Choose a reason for hiding this comment

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

self-review docs & examples

@@ -17,7 +15,7 @@ const getRequestToken = async (z, bundle) => {
oauth_callback: bundle.inputData.redirect_uri,
},
});
return querystring.parse(response.content);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trello is a great example of an app that can now use response.data

packages/legacy-scripting-runner/index.js Outdated Show resolved Hide resolved
packages/cli/src/tests/utils/build.js Show resolved Hide resolved
example-apps/babel/src/resources/recipe.js Show resolved Hide resolved
Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

looks awesome! Thanks for putting this all together.

example-apps/babel/src/resources/recipe.js Show resolved Hide resolved
packages/cli/README-source.md Outdated Show resolved Hide resolved
packages/cli/src/tests/utils/build.js Show resolved Hide resolved
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

Looks good except for the changes in legacy-scripting-runner. We'll want legacy-scripting-runner to be backward compatible with older versions of core.

packages/legacy-scripting-runner/index.js Outdated Show resolved Hide resolved
packages/legacy-scripting-runner/index.js Outdated Show resolved Hide resolved
packages/legacy-scripting-runner/index.js Outdated Show resolved Hide resolved
@FokkeZB
Copy link
Contributor Author

FokkeZB commented May 14, 2020

Thanks for the reviews @xavdid and @eliangcs - all feedback has been addressed.

@FokkeZB FokkeZB merged commit ed4e057 into master May 14, 2020
@FokkeZB FokkeZB deleted the responseData branch May 14, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants