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

Fix jsx error #252

Merged
merged 9 commits into from Nov 28, 2018
Merged

Fix jsx error #252

merged 9 commits into from Nov 28, 2018

Conversation

marvinhagemeister
Copy link
Collaborator

This PR changes quite a few compiler related thing to fix the nasty jsx error. The error is caused by breaking changes in the acorn plugin architecture. Being really complex and not maintained anymore I swapped it out in favour of a babel plugin.

  • Replace nodent with babel-plugin-transform-async-to-promises
  • Because the above needs babel, I replaced bublé with it (we may be able to keep both bublé and babel, so that babel only does the async/await transform)
  • Update rollup dependencies

Note that the compiled file sizes got significantly larger. I still need to investigate why that's the case. Perhaps someone else can have a look at it? /cc @developit

Fixes #74

@marvinhagemeister
Copy link
Collaborator Author

marvinhagemeister commented Nov 25, 2018

EDIT: This has been resolved, leaving it here for archival purposes.

The size difference is coming from babel-plugin-transform-async-to-promises.

Source:

async function foo(...args) {
  return [await two(...args), await two(...args)];
}

async function two(...args) {
  return args.reduce((total, value) => total + value, 0);
}

foo();

master (via nodent):

function n() {
  for (var n = [], r = arguments.length; r--; ) n[r] = arguments[r];
  return new Promise(function(r, t) {
    return r(
      n.reduce(function(n, r) {
        return n + r;
      }, 0),
    );
  });
}
module.exports = function() {
  for (var r = [], t = arguments.length; t--; ) r[t] = arguments[t];
  return new Promise(function(t, e) {
    return n.apply(void 0, r).then(function(u) {
      try {
        return n.apply(void 0, r).then(function(n) {
          try {
            return t([u, n]);
          } catch (n) {
            return e(n);
          }
        }, e);
      } catch (n) {
        return e(n);
      }
    }, e);
  });
};

Output from this PR:

const _async = (function() {
  try {
    if (isNaN.apply(null, {})) {
      return function(f) {
        return function() {
          try {
            return Promise.resolve(f.apply(this, arguments));
          } catch (e) {
            return Promise.reject(e);
          }
        };
      };
    }
  } catch (e) {}

  return function(f) {
    // Pre-ES5.1 JavaScript runtimes don't accept array-likes in Function.apply
    return function() {
      var args = [];

      for (var i = 0; i < arguments.length; i++) {
        args[i] = arguments[i];
      }

      try {
        return Promise.resolve(f.apply(this, args));
      } catch (e) {
        return Promise.reject(e);
      }
    };
  };
})();

function _await(value, then, direct) {
  if (direct) {
    return then ? then(value) : value;
  }

  value = Promise.resolve(value);
  return then ? value.then(then) : value;
}

const two = _async(function(...args) {
  return args.reduce((total, value) => total + value, 0);
});

const foo = function(...args) {
  return _await(two(...args), function(_two) {
    return _await(two(...args), function(_two2) {
      return [_two, _two2];
    });
  });
};

foo();

babel.config.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@marvinhagemeister
Copy link
Collaborator Author

@ForsakenHarmony can you retry the build? I can't reproduce the linting error that failed it locally

@developit
Copy link
Owner

developit commented Nov 26, 2018

@marvinhagemeister just need to merge 0d77987

@marvinhagemeister
Copy link
Collaborator Author

@ForsakenHarmony All the review comments have been addressed and I squashed the commits. I think the PR is ready for another review 👍

@wardpeet
Copy link
Collaborator

When trying to use closure-compiler on preact which still uses babel 6 to do it's transformations. Babel fails to transpile these files with this PR because @core/babel 7 is a dependency. It is used by the rollup-babel-plugin which means all non babel 7 compatible plugins will fail.
image

A few things to consider:

  1. we might want to do a major upgrade for the next release and warn users that we only support babel 7.
  2. Putting babel as a peer dependency can also be an option but we'll have to remove one dependency to bundle your library from our feature list.
  3. set babelrc option to false as a default but make it overridable through cli.

@marvinhagemeister
Copy link
Collaborator Author

@wardpeet Point 3) seems like the best choice for me. We didn't support custom babel configs before, so we should be ok to disable it.

@wardpeet
Copy link
Collaborator

sweet didn't knew that so if we just put babelrc: false we are good to go :)
https://github.com/rollup/rollup-plugin-babel#external-dependencies

@marvinhagemeister
Copy link
Collaborator Author

@wardpeet Added that line to the babel plugin. We weren't supporting this earlier because we didn't had babel included in the first place :)

@wardpeet
Copy link
Collaborator

@marvinhagemeister works like a charm now!

src/index.js Show resolved Hide resolved
@ForsakenHarmony
Copy link
Collaborator

Still wondering how fast-async would compare (it would not have the same problem, because jsx is handled elsewhere in babel)

@marvinhagemeister
Copy link
Collaborator Author

Did a few comparisons agaist fast-async/nodent. For standard async functions babel-plugin-transform-async-to-promises generates smaller output (~10-20b) and runs slightly faster. The latter can only be measured in benchmarks and probably has no real world effects.

Where fast-async has an advantage is nested for-loops that have an await statement some levels deep. In that case the output is significantly larger compared to the other babel plugin (about half the size). Interestingly the code runs only about half as fast as the other plugin. My guess is that it uses more function calls looking at a first glance. But I haven't really dug into what's going on.

Used the example benchmark code that is linked here, a few microbundle test cases and some of my own code.

@ForsakenHarmony
Copy link
Collaborator

Guessing you've tried all the options of fast async, well I can merge if you want

@marvinhagemeister
Copy link
Collaborator Author

That'd be awesome and I'd love if a release could be made in the following days 🎉 The jsx error is ever present on the CI for htm...

@ForsakenHarmony ForsakenHarmony merged commit cfd7428 into developit:master Nov 28, 2018
@marvinhagemeister
Copy link
Collaborator Author

@ForsakenHarmony woohoo, thanks for merging 🎉👍

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.

Error: Plugin 'jsx' not found
5 participants