refactor mark declaration, add PURE comments (#286) #309

modosc · 2017-08-04T23:22:11Z

(see also #301 for old discussion)

with the current code, this:

export function* test() {}
export function* abc() {}

compiles to this:

var _marked = [test, abc].map(regeneratorRuntime.mark);

export function test() {
  return regeneratorRuntime.wrap(function test$(_context) {
    while (1) {
      switch (_context.prev = _context.next) {
        case 0:
        case "end":
          return _context.stop();
      }
    }
  }, _marked[0], this);
}

export function abc() {
  return regeneratorRuntime.wrap(function abc$(_context2) {
    while (1) {
      switch (_context2.prev = _context2.next) {
        case 0:
        case "end":
          return _context2.stop();
      }
    }
  }, _marked[1], this);
}

because of the [test, abc].map call uglifyjs won't strip out test or abc if they're never actually imported (webpack does correctly identify them as non-imported though).

this pr changes two things:

the map call is unrolled and each call to regeneratorRuntime.mark has the magic #__PURE__ comment added to it so that uglify knows it can drop the code:

var _marked = /*#__PURE__*/regeneratorRuntime.mark(test),
    _marked2 = /*#__PURE__*/regeneratorRuntime.mark(abc);
export function test() {
  return regeneratorRuntime.wrap(function test$(_context) {
    while (1) {
      switch (_context.prev = _context.next) {
        case 0:
        case "end":
          return _context.stop();
      }
    }
  }, _marked, this);
}
export function abc() {
  return regeneratorRuntime.wrap(function abc$(_context2) {
    while (1) {
      switch (_context2.prev = _context2.next) {
        case 0:
        case "end":
          return _context2.stop();
      }
    }
  }, _marked2, this);
}

the magic comment is also added to function expressions, so this:

var foo = function* (){}

is compiled to this:

var foo = /*#__PURE__*/regeneratorRuntime.mark(function _callee() {
  return regeneratorRuntime.wrap(function _callee$(_context) {
    while (1) {
      switch (_context.prev = _context.next) {
        case 0:
        case "end":
          return _context.stop();
      }
    }
  }, _callee, this);
});

i added tests to verify the new variable declaration scheme. i haven't been able to get recast to return comments to me in a test though - i've verified that this works via bin/regenerator (to verify the comments are there) and by using it in my project with uglifyjs and verifying that unused generators are being stripped out.

Andarist · 2017-08-05T11:21:29Z

packages/regenerator-transform/src/visit.js

-    )
-  ]);
-
+  info.decl = t.variableDeclaration("var", [])


this looks odd, should it stay here in the code?

it's basically the same functionality as before which was to memoize the place the declarations were to go - i guess since it's so much smaller now it could go in the caller insetad?

Andarist · 2017-08-05T11:26:34Z

test/tests.transform.js

+  });
+});
+
+// TODO - how do i get recast to return comments?


i aint sure how what u are trying to achieve here exactly, ive managed to access comments like this:

recast.parse('var _marked = /*#__PURE__*/regeneratorRuntime.mark(test)').program.body[0].declarations[0].init.comments

maybe that helps you somehow

Andarist · 2017-08-05T11:28:00Z

test/tests.transform.js

+    const code = recast.print(transform(ast)).code;
+    assert.notStrictEqual(
+      code.indexOf(
+        'var a = /*#__PURE__*/Object.defineProperty( /*#__PURE__*/regeneratorRuntime.mark'


why the Object.defineProperty call here? didnt we ditch it?

sorry, copy pasta. will fix.

just updated all the tests, should be clearer now.

Andarist · 2017-08-05T11:31:26Z

packages/regenerator-transform/src/visit.js

+
+    // this can change from pass to pass if something else is altering our body
+    // so we have to look it up every time :/
+    let declarationsIndex = pp.node.body.findIndex((p) => {


maybe we could gather those declarations in context instead of inserting them right away and insert them all at once in program's exit visitor?

aint sure if thats better, just an idea

happy to look into this, it's probably best to not have to traverse the tree downward every time. i'm new to babel/ast stuff so it may take a bit to sort out.

im not a babel/ast expert either :P so i dont know what best practices are

just had such an idea how it should be possible to address the issue from the comment

I pushed another commit that contains some improvements for this logic.

Andarist · 2017-08-10T07:59:08Z

test/tests.transform.js

@@ -47,3 +47,102 @@ describe("_blockHoist nodes", function() {
    assert.deepEqual(names, ["hoistMe", "doNotHoistMe", "oyez"]);
  });
 });
+
+context("functions", function() {


are those good tests? I mean - they are highly coupled to the internal implementation, shouldnt we just test if the outcome is treeshakeable?

i'll add an uglify test.

👏 great work!

hah, thanks. is there anything else i can do to help expedite this merge? it makes a huge difference for me (and probably anyone else using redux-sagas).

I'll try to look at this today! Thanks so much to both of you for finding a simpler solution and perfecting it.

@modosc as maintainer of the redux-saga im a little bit surprised that this make u a huge difference. I thought this is more 'nice to have' and 'always better have this in place' rather than a game changer. Do u have any numbers for ur project? What will be tree shaken after this change?

I see when this might make a change when u have some shared saga utils module used between projects, but i think most dont

@Andarist lol, didn't realize that.

in our case we're generating api sagas automatically from swagger docs and grouping them by model. we've got at least one saga for every single version / endpoint / method so for some models we have >30 sagas in a single file. most of them aren't used but due to this issue using a single saga from a file pulls in all the others.

in retrospect i probably overstated the value of this to the average user - in normal circumstances i can't imagine having that many sagas sitting around that aren't actually used (although i can imagine situations with code-spitting where you end up bundling an entire module because you want access to a single saga inside of it).

in our case we're generating api sagas automatically from swagger docs and grouping them by model.

metaprogramming level hard :D would love to see this in action

although i can imagine situations with code-spitting where you end up bundling an entire module because you want access to a single saga inside of it

yeah, thats true, however i see the issue only for 'util' saga files, others are rather grouped by some kind of functionality and they rather work only together

benjamn · 2017-08-15T19:11:59Z

Thanks @modosc and @Andarist for iterating on this and making it so much simpler!

Now to publish these packages with Lerna for the first time…

benjamn · 2017-08-15T19:46:48Z

You can see these changes live in the sandbox now! http://facebook.github.io/regenerator/

Andarist · 2017-08-15T20:41:14Z

wohoooooo! 🎉

refactor mark declaration, add __PURE__ comments

f23e8cf

facebook-github-bot added the CLA Signed label Aug 4, 2017

This was referenced Aug 4, 2017

refactor mark declaration to make it more tree-shakable (#286) #301

Closed

Tree-shaking transformed generator functions #286

Closed

Andarist reviewed Aug 5, 2017

View reviewed changes

cleanup / fix tests

3142a3f

Andarist reviewed Aug 10, 2017

View reviewed changes

add actual dead code removal test with uglify

215c1ef

benjamn self-assigned this Aug 11, 2017

benjamn added 2 commits August 15, 2017 14:33

Move uglify-js to devDependencies and other minor tweaks.

d1ce442

Move more logic into getMarkedFunctionId helper function.

4848531

benjamn merged commit 10cbffe into facebook:master Aug 15, 2017

benjamn mentioned this pull request Aug 15, 2017

Update regenerator-transform to version 0.10.0. babel/babel#6109

Closed

modosc mentioned this pull request Aug 15, 2017

Update regenerator babel/babel#6111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor mark declaration, add PURE comments (#286) #309

refactor mark declaration, add PURE comments (#286) #309

modosc commented Aug 4, 2017

Andarist Aug 5, 2017

modosc Aug 5, 2017

Andarist Aug 5, 2017

Andarist Aug 5, 2017

modosc Aug 5, 2017

modosc Aug 5, 2017

Andarist Aug 5, 2017

modosc Aug 5, 2017

Andarist Aug 6, 2017

benjamn Aug 15, 2017

Andarist Aug 10, 2017

modosc Aug 10, 2017

modosc Aug 10, 2017

Andarist Aug 11, 2017

modosc Aug 11, 2017

benjamn Aug 11, 2017

Andarist Aug 11, 2017

modosc Aug 11, 2017

Andarist Aug 11, 2017

benjamn commented Aug 15, 2017

benjamn commented Aug 15, 2017

Andarist commented Aug 15, 2017

refactor mark declaration, add __PURE__ comments (#286) #309

refactor mark declaration, add __PURE__ comments (#286) #309

Conversation

modosc commented Aug 4, 2017

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

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

benjamn commented Aug 15, 2017

benjamn commented Aug 15, 2017

Andarist commented Aug 15, 2017

refactor mark declaration, add PURE comments (#286) #309

refactor mark declaration, add PURE comments (#286) #309