Replace deprecated `requests` with `node-fetch` #191

rmeritz · 2020-06-18T17:22:53Z

Requests has been deprecated: Request’s Past, Present and Future request/request#3142
There are lot of options for replacements: Alternative libraries to request request/request#3143
Went with node-fetch because it is the most lightweight: https://github.com/node-fetch/node-fetch
Use abort-controller to add back the timeout
https://github.com/node-fetch/node-fetch#request-cancellation-with-abortsignal
When we get a request back cancel the timeout because we might need to
recur. https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/clearTimeout
Tested that node-fetch replacement actually works end-to-end by running the test/test_files/index.html
In order to test against the latest changes I regenerated thebrowserifyed js with node_modules/grunt/bin/grunt browserify and copied the built file to the test directly. I'm unclear what the process should be for when airtable.browser.js should be updated. I didn't check in the built version, because I assume that is only updated when I release is done. And explanation of how and when to regenerate the airtable.browser.js would be great to add to the README, but I'm not sure what the process is.

rmeritz · 2020-06-18T18:15:41Z

So it seems that node-fetch doesn't work with node 8 which is now in LTS. https://nodejs.org/en/blog/release/v8.9.0/

I've replicated all the falling tests on the node 8 build locally and it is a real problem with the library.

Looking at the commit history it seems like this library had only be supporting node 10+ but then a decision was explicitly made to support node 8+. 31fd0a0

@kasrak - Do we definitely want to support node 8 moving forward? I can close this in favor of another solution if so. Sorry I didn't catch this issue earlier, but at least we have CI!

rmeritz · 2020-06-18T19:30:59Z

@kasrak - So it turns out the probably wasn't with node-fetch but with my own code changes because I was using finally which wasn't in node 8. So you can ignore what I said about node 8. This should be good to go now.

kasrak

Hm, this causes the browser build (airtable.browser.js) to go from ~200KB to 2MB which seems fishy.

kasrak · 2020-06-23T03:21:01Z

In order to test against the latest changes I regenerated thebrowserifyed js with node_modules/grunt/bin/grunt browserify and copied the built file to the test directly. I'm unclear what the process should be for when airtable.browser.js should be updated. I didn't check in the built version, because I assume that is only updated when I release is done. And explanation of how and when to regenerate the airtable.browser.js would be great to add to the README, but I'm not sure what the process is.

Yep, the workflow is to manually generate a new airtable.browser.js as part of a release (e.g. 31fd0a0). But in this case let's keep checking in the built bundle to verify the size as part of this code review. Once we're happy with it, we'll uncheck-in those changes and land this PR without any changes to airtable.browser.js

Use abort-controller as suggested by node-fetch https://github.com/node-fetch/node-fetch#request-cancellation-with-abortsignal When we get a request back cancel the timeout because we might need to recur. https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/clearTimeout

- Tested that `node-fetch` replacement actually works by running the `test/test_files/index.html` - In order to test againast the latest changes I regenerated the browserifyed js with `node_modules/grunt/bin/grunt browserify`. - I copied the built file to the test directly. - I'm unclear what the process is for when `airtable.browser.js` should be updated. I didn't check in the built version, because I assume that is only updated when I release is done, but I'm not clear what the process is. That would be something that would be great to clarify in the README.

- Fix sending a signal so that it actually aborts

- Remove the use of finally. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally#Browser_compatibility - Increase timeouts because test became flakely otherwise

rmeritz · 2020-06-23T19:05:22Z

@kasrak - I'm working on figuring out a solution to this. In order to move forward it would be helpful to know what browsers (and versions) this library should support. It would help me figure out what polyfills I need to add (or not) and it would help future users have a clear understanding of if the library will work for them or not.

I see we are already polyfilling promise even though it has wide support (except for IE). https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#Browser_compatibility Should that be taken as an explicate decision to support at least IE?

I need to determine if I need to polyfill fetch and abortController as well. https://developer.mozilla.org/en-US/docs/Web/API/AbortController https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API#Browser_compatibility

The deprecated requests module was also being used in `base#makeRequests` even though it was no longer in the list of installed packages in package.json browserify was pulling it it which was the root cause of the package being so bloated (because I was not longer replaceing requests with xhr). This replaces `request` with `node-fetch` in `makeRequest`. `makeRequest`s isn't not actually used anywhere. All API calls actually are going through `runAction` instead and I think maybe `makeRequest`s should just be removed but in order to do that we need to get more clarity around what is public vs private.

- Regenerate the the airtable.browser.js via grunt and ensure that it works via the python server - Replace `node-fetch` and `abort-controller` packages with the build in versions on the window for the browser build - Maybe they should instead be replace by polyfills. There is a lack of clarity of what browsers needs to be supported. - Both fetch and abortController are widely supported by many browsers so I think the polyfills can be skipped. https://developer.mozilla.org/en-US/docs/Web/API/AbortController#Browser_compatibility https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API#Browser_compatibility

rmeritz · 2020-06-24T20:16:34Z

@kasrak - I figured out what was causing the bloat of the built package size. It turns out there was a second use of the request package in base#makeRequest that I was missing. And even though I had removed the request library from package.json browserify was pulling it back in. I've removed the use of request in base#makeRequest with fetch as well. And now the built library is back down to 220K.

This is now working but it would still be great to address the question I asked about what browser versions we want to support. I didn't end up pulling in pollyfills for fetch or abortController because they are both widely supported and I didn't want to add bloat. It would be very helpful to me and all future users and maintainers to clearly lay out in the README what versions are supported.

Can you clarify what versions we support so I can determine if we need to add polyfills or not?

Maybe that would also allow us to pull out the Promise polyfill library as one user already asked for. #179 I would be happy to make that change as well.

Also, one reason I missed the use of request in base#makeRequest is that makeRequest is never actually used anywhere. It is technically possible to access it but 100% of API calls are filtered through runAction which does practically the same thing as makeRequest. Can I just remove makeRequest or deprecate it? Do we consider it a public API? I cannot imagine a user wanting to use it and it isn't part of the documentation.

Speaking of the public API, when working making changes to this library and writing tests it came up over and over again that there isn't a clear distinction documented anywhere of what is a public and what is a private interface. I think it would very helpful to the long term maintainability of the project to provide clarity around this. I would be more than happy to add jdocs to make this clear (using the public API docs as my guide) if you agree that that would be a useful path forward. Do you think it makes sense for me to go ahead and do this? (Esp ahead of the Typescript changes).

rmeritz · 2020-06-24T20:18:00Z

Also, I cannot add linked issues, but this should close #153 when merged.

kasrak · 2020-06-26T03:17:02Z

This is now working but it would still be great to address the question I asked about what browser versions we want to support. I didn't end up pulling in pollyfills for fetch or abortController because they are both widely supported and I didn't want to add bloat. It would be very helpful to me and all future users and maintainers to clearly lay out in the README what versions are supported.

Let's match the supported browser list for Airtable (https://support.airtable.com/hc/en-us/articles/217990018), which is currently:

Chrome 67 or higher
Firefox 52 or higher
Safari 10 or higher
Edge 25 or higher

So we can drop support for IE, drop the Promise polyfill (maybe in a separate PR?), and do a major version bump when we release the next version of Airtable.js

Can I just remove makeRequest or deprecate it? Do we consider it a public API?

Let's leave it in for now, it's handy for making more custom requests to the API.

there isn't a clear distinction documented anywhere of what is a public and what is a private interface.

By convention, any identifiers beginning with _ are private. Let's chat separately about ways to make that more clear.

Does any of the above require changes to this PR? Let me know if not and I'll review the code changes. Thanks!

rmeritz · 2020-06-26T16:53:50Z

Does any of the above require changes to this PR? Let me know if not and I'll review the code changes. Thanks!

Yes. I have to make one more set of changes. Thanks so much for providing clear requirements on browser support. It does mean that I will need to add 2 polyfills though: one for fetch just for Safari 10.0 and one for AbortController for Safari 10 - 11 and Firefox 52 - 56.

drop the Promise polyfill (maybe in a separate PR?), and do a major version bump when we release the next version of Airtable.js

I'm drop the promise polyfill in a follow-up PR.

By convention, any identifiers beginning with _ are private. Let's chat separately about ways to make that more clear.

I think that is quite clear. But the API documented in the developer docs for airtable.js (the ones generated per base) only shows the use of a tiny tiny percentage of what according to the convention is technically public. It would be helpful to the long-term maintainability of the library to only commit to a small externally public API -- meaning that there would be a distinction between methods that a are private to a file (using underscore) and methods that are private to external use. Anyway, this definitely doesn't have to be answered in this PR at all. I just thought I would mention it while it was top of mind.

- Add AbortController polyfill for Safari 10 - 11 and Firefox 52 - 56 - Was going to add fetch polyfill but it is only missing from Safari 10.0. It is in Safari 10.1 and all other browsers that need to be supported. All Safari before 10.1 is only 0.26% of all browsers usage. It is not possible to test Safari 10.0 on Browserstack and the complexity and size cost of adding the polyfill for this one minor version is large, so it was skipped it. Including the one polyfill brings the size of the built package to 232K up from 220K.

rmeritz · 2020-06-26T23:12:23Z

@kasrak - I added the polyfill needed and tested it works with Browserstack in the older browsers, so you can review it again.

minicat

looks mostly good too me, had a few questions!

minicat · 2020-07-14T10:07:27Z

README.md

@@ -18,6 +18,7 @@ To install airtable.js in a node project:

    npm install airtable

+Airtable.js should work with Node 10 and above.


nit: maybe is compatible with instead of should work? So the wording is a bit more confidence inspiring 😅

minicat · 2020-07-14T10:07:35Z

README.md

@@ -33,6 +34,8 @@ Edit `test/test_files/index.html` - put your `BASE_ID` and `API_KEY` (Be careful

 Then open http://localhost:8000/ in your browser.

+Airtable.js should work with browsers supported by Airtable App.


is compatible with and Airtable web app

Fixed as well.

minicat · 2020-07-14T13:13:18Z

test/test_files/airtable.browser.js

@@ -1,4 +1,17 @@
 require=(function(){function r(e,n,t){function o(i,f){if(!n[i]){if(!e[i]){var c="function"==typeof require&&require;if(!f&&c)return c(i,!0);if(u)return u(i,!0);var a=new Error("Cannot find module '"+i+"'");throw a.code="MODULE_NOT_FOUND",a}var p=n[i]={exports:{}};e[i][0].call(p.exports,function(r){var n=e[i][1][r];return o(n||r)},p,p.exports,r,e,n,t)}return n[i].exports}for(var u="function"==typeof require&&require,i=0;i<t.length;i++)o(t[i]);return o}return r})()({1:[function(require,module,exports){


remember to revert these changes before landing!

I've reverted it.

minicat · 2020-07-14T13:18:43Z

test/base.test.js

@@ -346,32 +355,26 @@ describe('Base', function() {
            });

            it('retries 429s until success', function() {
-                const realSetTimeout = setTimeout;
-
-                jest.useFakeTimers();


why was this changed? & number of requests changed from 3 to 2

It was a few weeks ago, so I'm not 100% sure. But I think I just changed it from 2 to 3 requests to make the tests faster.

As work that procceded, I wrote a bunch of new timeout tests (to get 100% coverage) that use the actual timeouts and I think I just made this test more consistent with the other.

I think I did this work because I ran into an issue with how I passed the timeout signal with the new node fetch library which caused me to dig in with these tests more.

Ahh i see - sounds reasonable, thanks!

minicat · 2020-07-14T13:23:34Z

lib/base.js

    if ('body' in options && _canRequestMethodIncludeBody(method)) {
-        requestOptions.body = options.body;
+        requestOptions.body = JSON.stringify(options.body);


im assuming the JSON.stringify was previously handled by the request library?

minicat · 2020-07-14T13:25:16Z

test/test_helpers.js

@@ -143,6 +143,7 @@ function getMockEnvironmentAsync(options) {
                        airtable: new Airtable({
                            apiKey: 'key123',
                            endpointUrl: 'http://localhost:' + testServerPort,
+                            requestTimeout: 1000,


nit: maybe add a comment mentioning this is required for the timeout tests

rmeritz · 2020-07-14T21:30:16Z

@minicat - Thanks for taking a look. I think I addressed all your comments and answered your questions now.

Also, if this is ready to land. I'm 100% sure that I don't have permission do it and someone will need to do it internally.

README.md

minicat · 2020-07-15T09:51:49Z

test/base.test.js

@@ -346,32 +355,26 @@ describe('Base', function() {
            });

            it('retries 429s until success', function() {
-                const realSetTimeout = setTimeout;
-
-                jest.useFakeTimers();


Ahh i see - sounds reasonable, thanks!

minicat

thanks! A few spelling nits but otherwise looks good to go

Co-authored-by: Emma Yeap <6092008+minicat@users.noreply.github.com>

rmeritz · 2020-07-16T17:08:21Z

@minicat - I committed spelling/grammar changes you caught. Thanks!

- Bodys sent with HEAD/GET requests should not cause TypeErrors instead just add a console.warning.

`runAction` exposes a `Response` object in its callback. The old `request` library had a slightly different interface than the one for the `Response` object for `node-fetch`. Specifically, both have `body` defined by `request` had `body` as the parsed json body. Add the parsed json body to the `Response` object returned by runAction. `body` has no `setter` and only a `getter` so a new object needs to be created instead.

The Promise polyfill is not needed with the outlined techinal requirements for node or browser support. Closes Airtable#179 A follow up to: Airtable#191

The Promise polyfill is not needed with the outlined techinal requirements for node or browser support. Closes Airtable/airtable.js#179 A follow up to: Airtable/airtable.js#191

kasrak suggested changes Jun 23, 2020

View reviewed changes

rmeritz added 7 commits June 23, 2020 11:02

Add test for custom user agent header if browser

5cee15c

Add test of retry if hitting rate limit

bec747d

Replace deprecated request library with node-fetch

bd98f7f

Add test of timeout abort

a5f960c

- Fix sending a signal so that it actually aborts

Ensure fetch works in node 8

0bb0460

- Remove the use of finally. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally#Browser_compatibility - Increase timeouts because test became flakely otherwise

rmeritz force-pushed the node-fetch-rmeritz branch from d7d6235 to 406e00f Compare June 24, 2020 19:58

rmeritz force-pushed the node-fetch-rmeritz branch from 406e00f to 8b92079 Compare June 24, 2020 20:02

rmeritz requested a review from kasrak June 24, 2020 20:19

rmeritz added 2 commits June 26, 2020 13:35

Add docs to README about requirements

99bb192

rmeritz force-pushed the node-fetch-rmeritz branch from e35bfed to a24d8ec Compare June 26, 2020 23:11

rmeritz mentioned this pull request Jun 30, 2020

Configure airtable.js work as a typescript library bocoup/airtable.js#1

Closed

kasrak requested a review from emmettnicholas July 6, 2020 17:17

richsinn requested a review from minicat July 8, 2020 18:34

minicat reviewed Jul 14, 2020

View reviewed changes

Minor fixes from review

4672267

minicat reviewed Jul 15, 2020

View reviewed changes

rmeritz and others added 2 commits July 16, 2020 10:07

Fix grammer in README

d1f8de1

Co-authored-by: Emma Yeap <6092008+minicat@users.noreply.github.com>

Add additional spelling fix in README.md

ce08308

Co-authored-by: Emma Yeap <6092008+minicat@users.noreply.github.com>

rmeritz added 5 commits July 17, 2020 13:31

Add test for requests with empty bodies

f84c989

Preserve behavior from request library

501310a

- Bodys sent with HEAD/GET requests should not cause TypeErrors instead just add a console.warning.

Add test to handle errors with empty bodies

32152b6

Preserve error handling behavior for bodyless err

dbe0992

minicat approved these changes Jul 23, 2020

View reviewed changes

minicat merged commit 5d3948d into Airtable:master Jul 23, 2020

rmeritz added a commit to bocoup/airtable.js that referenced this pull request Jul 23, 2020

Remove Promise Polyfill

d7554c8

The Promise polyfill is not needed with the outlined techinal requirements for node or browser support. Closes Airtable#179 A follow up to: Airtable#191

rmeritz mentioned this pull request Jul 23, 2020

Remove Promise Polyfill #195

Merged

This was referenced Jul 27, 2020

Replacement of 'request' HTTP library. #167

Closed

Replace request lib #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated `requests` with `node-fetch` #191

Replace deprecated `requests` with `node-fetch` #191

rmeritz commented Jun 18, 2020

rmeritz commented Jun 18, 2020

rmeritz commented Jun 18, 2020

kasrak left a comment

kasrak commented Jun 23, 2020

rmeritz commented Jun 23, 2020

rmeritz commented Jun 24, 2020

rmeritz commented Jun 24, 2020 •

edited

Loading

kasrak commented Jun 26, 2020 •

edited

Loading

rmeritz commented Jun 26, 2020

rmeritz commented Jun 26, 2020

minicat left a comment

minicat Jul 14, 2020

rmeritz Jul 14, 2020

minicat Jul 14, 2020

rmeritz Jul 14, 2020

minicat Jul 14, 2020

rmeritz Jul 14, 2020

minicat Jul 14, 2020

rmeritz Jul 14, 2020

minicat Jul 15, 2020

minicat Jul 14, 2020

rmeritz Jul 14, 2020

minicat Jul 14, 2020

rmeritz Jul 14, 2020

rmeritz commented Jul 14, 2020 •

edited

Loading

minicat Jul 15, 2020

minicat left a comment

rmeritz commented Jul 16, 2020

		@@ -18,6 +18,7 @@ To install airtable.js in a node project:

		npm install airtable

		Airtable.js should work with Node 10 and above.

		@@ -33,6 +34,8 @@ Edit `test/test_files/index.html` - put your `BASE_ID` and `API_KEY` (Be careful

		Then open http://localhost:8000/ in your browser.

		Airtable.js should work with browsers supported by Airtable App.

		@@ -1,4 +1,17 @@
		require=(function(){function r(e,n,t){function o(i,f){if(!n[i]){if(!e[i]){var c="function"==typeof require&&require;if(!f&&c)return c(i,!0);if(u)return u(i,!0);var a=new Error("Cannot find module '"+i+"'");throw a.code="MODULE_NOT_FOUND",a}var p=n[i]={exports:{}};e[i][0].call(p.exports,function(r){var n=e[i][1][r];return o(n\|\|r)},p,p.exports,r,e,n,t)}return n[i].exports}for(var u="function"==typeof require&&require,i=0;i<t.length;i++)o(t[i]);return o}return r})()({1:[function(require,module,exports){

Replace deprecated requests with node-fetch #191

Replace deprecated requests with node-fetch #191

Conversation

rmeritz commented Jun 18, 2020

rmeritz commented Jun 18, 2020

rmeritz commented Jun 18, 2020

kasrak left a comment

Choose a reason for hiding this comment

kasrak commented Jun 23, 2020

rmeritz commented Jun 23, 2020

rmeritz commented Jun 24, 2020

rmeritz commented Jun 24, 2020 • edited Loading

kasrak commented Jun 26, 2020 • edited Loading

rmeritz commented Jun 26, 2020

rmeritz commented Jun 26, 2020

minicat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmeritz commented Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

minicat left a comment

Choose a reason for hiding this comment

rmeritz commented Jul 16, 2020

Replace deprecated `requests` with `node-fetch` #191

Replace deprecated `requests` with `node-fetch` #191

rmeritz commented Jun 24, 2020 •

edited

Loading

kasrak commented Jun 26, 2020 •

edited

Loading

rmeritz commented Jul 14, 2020 •

edited

Loading