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

use babel-plugin-transform-async-to-promises #579

Closed
wants to merge 3 commits into from
Closed

Conversation

apaleslimghost
Copy link
Member

@apaleslimghost apaleslimghost commented Sep 9, 2019

fixes #23

@i-like-robots
Copy link
Contributor

I've added a "do not merge" label to this in case we do need to release any 0.2.0 patches (because I don't know if we really need to implement a branching model right now)

@i-like-robots
Copy link
Contributor

i-like-robots commented Sep 10, 2019

I gave this a quick test and the results are good!

Before:

-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users    25K 10 Sep 10:59 babel-runtime.js
-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users   386B 10 Sep 10:59 external-lib-one.js
-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users   633B 10 Sep 10:59 external-lib-two.js
-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users   2.2K 10 Sep 10:59 scripts.js
-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users   6.1K 10 Sep 10:59 webpack-runtime.js

After:

-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users   392B 10 Sep 11:01 external-lib-one.js
-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users   624B 10 Sep 11:01 external-lib-two.js
-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users   1.8K 10 Sep 11:01 scripts.js
-rw-r--r--  1 matt.hinchliffe  ADFT\Domain Users   6.1K 10 Sep 11:01 webpack-runtime.js

So in this case we were able to remove the entire Babel runtime/regenerator chunk \0/

The output also looks much better...

Before:

var main =
/*#__PURE__*/
function () {
  var _ref = asyncToGenerator_default()(
  /*#__PURE__*/
  regenerator_default.a.mark(function _callee() {
    var response, result;
    return regenerator_default.a.wrap(function _callee$(_context) {
      while (1) {
        switch (_context.prev = _context.next) {
          case 0:
            _context.next = 2;
            return fetch('https://www.ft.com/world?format=json');

          case 2:
            response = _context.sent;

            if (!response.ok) {
              _context.next = 7;
              break;
            }

            _context.next = 6;
            return response.json();

          case 6:
            result = _context.sent;

          case 7:
            console.log(result);

          case 8:
          case "end":
            return _context.stop();
        }
      }
    }, _callee);
  }));

  return function main() {
    return _ref.apply(this, arguments);
  };
}();

After:

var main = _async(function () {
  return _await(fetch('https://www.ft.com/world?format=json'), function (response) {
    var result;
    return _invoke(function () {
      if (response.ok) {
        return _await(response.json(), function (_response$json) {
          result = _response$json;
        });
      }
    }, function () {
      console.log(result);
    });
  });
});

There are some options to disable the plugin's helpers and use "raw" Promise.whatever calls instead, do you think it's worth investigating those at all?

@i-like-robots
Copy link
Contributor

I was demonstrating this plugin to Adam on Tuesday and we tried out the different output options. We noticed that the helper functions are injected into each module which needs them which can lead to repetition. There is an option to extract these into a separate runtime but this is almost as big as Regenerator! Setting inlineHelpers: true (I don't get why it's called this 🤔) to output vanilla promises gave us the most readable and debuggable output.

@adambraimbridge
Copy link
Contributor

adambraimbridge commented Sep 25, 2019

I grok not, but perhaps this might be worth looking at: https://github.com/MatAtBread/fast-async

Latest commit was last year, so since then babel-plugin-transform-async-to-promises is better maintained. I just thought I'd mention it.

@i-like-robots
Copy link
Contributor

i-like-robots commented Sep 26, 2019

In the original issue (#23) we considered implementing fast-async but we initially we hesitated because the benefits appeared to be overblown (there was a bit of a tiff between the maintainer and a similar package where they both argued their solution was faster/smaller/better).

As it turns out the package Bren has implemented is probably closest to what we want as it produces reasonable output by using native promises rather than an approximation of generators.

@adambraimbridge
Copy link
Contributor

Thanks for explaining, Matt. 👍

@i-like-robots
Copy link
Contributor

I've added a Q&A label to this, I will check this out and try building a real app with along with #574 🔎

@i-like-robots
Copy link
Contributor

i-like-robots commented Oct 14, 2019

Unfortunately this plugin seems to stumble with some of our code. I think it's a bug with the plugin rather than our code, for example this input:

// https://github.com/Financial-Times/n-syndication/blob/master/src/js/get-user-status.js
export default async function getUserStatus () {
	const url = '/syndication/user-status';
	const options = {
		credentials: 'include'
	};

	try {
		const response = await fetch(url, options);
		if (response.ok) {
			return response.json();
		} else {
			...
		}
	} catch (err) {
		...
	}
}

Is output as:

var getUserStatus = function getUserStatus() {
  try {
    url = '/syndication/user-status'; // undeclared variable
    var options = {
      credentials: 'include'
    };
    return Promise.resolve(_catch(function () {
      return fetch(url, options).then(function (_fetch) {
        var _exit = false;
        response = _fetch; // undeclared variable
          ...
      });
    }, function(err) {
        ...
    });
  } catch (e) {
    return Promise.reject(e);
  }
}

🤔

@i-like-robots
Copy link
Contributor

I've created a minimum test case for this issue here: https://github.com/i-like-robots/broken-babel-async-await-transform-test-case and reported it here: rpetrich/babel-plugin-transform-async-to-promises#45

@i-like-robots
Copy link
Contributor

From the plugin author:

Babel's scope tracking got more strict in babel 7.6 and broke the tracking of variables across functions; this is now remedied in master and will be in the next release.

@NickColley
Copy link
Contributor

I just ran your reduced test case and it looks like the newer version outputs correctly:

var getStatus = function getStatus() {
  try {
    var response;
    var url = "/path/to/status";
    return Promise.resolve(_catch(function () {
      return Promise.resolve(fetch(url)).then(function (_fetch) {
        response = _fetch;

        if (response.ok) {
          return response.json();
        } else {
          throw Error("".concat(url, " responded with ").concat(response.status));
        }
      });
    }, function (error) {
      console.error(error);
    }));
  } catch (e) {
    return Promise.reject(e);
  }
};

Does this mean this can continue and is no longer blocked?

Base automatically changed from master to main February 8, 2021 12:49
@emortong emortong requested a review from a team as a code owner February 8, 2021 12:49
@apaleslimghost
Copy link
Member Author

closing this as we've clearly been fine without it for three years and also i'm pretty sure async/await is supported in all our target browsers now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants