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

Add "importRuntime" option to "transform-runtime" #356

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nicolo-ribaudo
Copy link
Contributor

When "importRuntime" is true, regenerator doesn't assume that
regeneratorRuntime is available globally, but it injects an import
statement (or require, depending on the source type)

Example output:

var _regeneratorRuntime = require("regenerator-runtime");

var _marked =
/*#__PURE__*/
_regeneratorRuntime.mark(f);

function f() {
  return _regeneratorRuntime.wrap(function f$(_context) {
    while (1) switch (_context.prev = _context.next) {
      case 0:
      case "end":
       return _context.stop();
    }
  }, _marked, this);
}

I had to update the Babel deps because I hit babel/babel#8508 and babel/babel#8722.

When "importRuntime" is true, regenerator doesn't assume that
`regeneratorRuntime` is available globally, but it injects an `import`
statement (or `require`, depending on the source type)
package.json Outdated
"regenerator-transform": "^0.13.3",
"regenerator-preset": "file:packages/regenerator-preset",
"regenerator-runtime": "file:packages/regenerator-runtime",
"regenerator-transform": "file:packages/regenerator-transform",
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 changes were made by lerna I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we revert the lerna changes?

Copy link
Collaborator

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

This seems like the right direction, though I have one substantive suggestion.

export function runtimeProperty(name) {
const runtimeNames = new WeakMap();

export function runtimeProperty(name, scope, opts) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that runtimeProperty takes options and does more than just construct a member expression, I think we should move it out of util.js and make it a method of the Emitter class. That way we won't need to pass the options explicitly, since they're stored as a property of the Emitter object now. From what I can tell, everywhere runtimeProperty was previously called, we have access to an Emitter instance, so this change should be pretty straightforward. What do you think @nicolo-ribaudo?

@loganfsmyth
Copy link
Contributor

To throw it out there, I don't have a problem with landing this if you want, but I don't think it's something we'd recommend people use on the Babel side since plugin-transform-runtime also handles injecting a reference to the runtime and is already very recommended in Babel 7.

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

Successfully merging this pull request may close these issues.

None yet

4 participants