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

Perhaps `babel-plugin-transform-runtime` should put polyfills on local scope? #6932

Closed
jedwards1211 opened this Issue Nov 29, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@jedwards1211

jedwards1211 commented Nov 29, 2017

TL;DR

eval('Map') should work even with transform-runtime; all transform-runtime needs to do is
put (essentially) var Map = require('babel-runtime/core-js/Map') at the top of the file instead of altering references to Map inline.

Likewise for any other ES2015 globals.

Change request

I just ran into an issue with babel-plugin-flow-runtime together with babel-plugin-transform-runtime.

Currently transform-runtime converts new Map() to new _map2.default() and likewise for other ES2015 globals.
But flow-runtime is trying to check that something is an instance of Map (referenced via the string 'Map', which transform-runtime can't convert to _map2.default).
Wouldn't it be better to put var Map = _map2.default at the top of the module so that the local scope looks like an ES2015 environment, even though babel-polyfill isn't being used?

Input Code

// @flow

class Foo {
  bar: Map<string, any> = new Map()

  getValue(name: string) {
    return this.bar.get(name)
  }
}

new Foo().getValue('hello')

Babel/Babylon Configuration (.babelrc, package.json, cli command)

{
  "presets": [["env", {"targets": {"node": "current"}}], "flow"],
  "plugins": [
    "transform-decorators-legacy",
    ["flow-runtime"],
    "transform-runtime",
    "transform-class-properties",
  ],
}

Expected Behavior

Given code runs without runtime errors.

Current Behavior

Running the code outputs the following error:

$ npm-do babel-node temp.js
/Users/andy/iron-pi-webapp/node_modules/flow-runtime/dist/flow-runtime.js:9331
        throw error;
        ^

RuntimeTypeError: Default value for property Foo.bar must be an instance of Map

Expected: Map

Actual: Map

Possible Solution

I think setting var Map = _map2.default at the top instead of converting new Map() to new _map2.default() would solve the problem (and potentially other similar problems).

Context

The transpiled code (truncated) is as follows. The runtime type check fails because flow-runtime gets Map from the local context (a reasonable expectation for an ES2015 environment), which is a different object than _map2.default.

var _map = require('babel-runtime/core-js/map');

var _map2 = _interopRequireDefault(_map);

...

let Foo = (_dec = _flowRuntime2.default.annotate(_flowRuntime2.default.class('Foo', _flowRuntime2.default.property('bar', _flowRuntime2.default.ref('Map', _flowRuntime2.default.string(), _flowRuntime2.default.any())), _flowRuntime2.default.method('getValue', _flowRuntime2.default.param('name', _flowRuntime2.default.string())))), _dec2 = _flowRuntime2.default.decorate(_flowRuntime2.default.ref('Map', _flowRuntime2.default.string(), _flowRuntime2.default.any())), _dec(_class = (_class2 = class Foo {
  constructor() {
    _initDefineProp(this, 'bar', _descriptor, this);
  }

  getValue(name) {
    let _nameType = _flowRuntime2.default.string();

    _flowRuntime2.default.param('name', _nameType).assert(name);

    return this.bar.get(name);
  }
}, (_descriptor = _applyDecoratedDescriptor(_class2.prototype, 'bar', [_dec2], {
  enumerable: true,
  initializer: function () {
    return new _map2.default();
  }
})), _class2)) || _class);

Your Environment

    "babel-core": "^6.26.0",
    "babel-plugin-flow-runtime": "^0.15.0",
    "babel-plugin-transform-class-properties": "^6.24.1",
    "babel-plugin-transform-decorators-legacy": "^1.3.4",
    "babel-plugin-transform-runtime": "^6.23.0",
    "babel-preset-env": "^1.6.1",
    "babel-preset-flow": "^6.23.0",
@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Nov 29, 2017

Collaborator

Hey @jedwards1211! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

Collaborator

babel-bot commented Nov 29, 2017

Hey @jedwards1211! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Nov 29, 2017

Member

Currently transform-runtime converts new Map() to new _map2.default() and likewise for other ES2015 globals.

To clarify, this is two separate processes.

new Map()

is converted to

import Map from "..."
new Map()

as part of transform-runtime, and then the module transform, which has no knowledge of anything, converts it to

var mod = interop(require("..."));
new mod.default()

So given that, doing a general transform of

var Map = interop(require("...")).default;
new Map()

would require knowing ahead of time that the imported default value will never change, which isn't something that the module transform has a way of knowing, since ES6 module syntax allows generic live bindings.

Member

loganfsmyth commented Nov 29, 2017

Currently transform-runtime converts new Map() to new _map2.default() and likewise for other ES2015 globals.

To clarify, this is two separate processes.

new Map()

is converted to

import Map from "..."
new Map()

as part of transform-runtime, and then the module transform, which has no knowledge of anything, converts it to

var mod = interop(require("..."));
new mod.default()

So given that, doing a general transform of

var Map = interop(require("...")).default;
new Map()

would require knowing ahead of time that the imported default value will never change, which isn't something that the module transform has a way of knowing, since ES6 module syntax allows generic live bindings.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Nov 29, 2017

@loganfsmyth gotcha, okay. Well I guess this is a flaw in the design of babel-plugin-flow-runtime then. Hopefully it can reference Map directly instead of essentially doing eval('Map') at runtime.

jedwards1211 commented Nov 29, 2017

@loganfsmyth gotcha, okay. Well I guess this is a flaw in the design of babel-plugin-flow-runtime then. Hopefully it can reference Map directly instead of essentially doing eval('Map') at runtime.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Nov 29, 2017

Member

Yeah, the two main cases where we rename are

  • Modules, rewriting variables to property accesses.
  • Variable renames for function naming:
    var Map = window.Map;
    
    var obj = {
      Map(){
       	console.log(Map);
      },
    };
    
    becomes
    var _Map = window.Map;
    var obj = {
      Map: function Map() {
        console.log(_Map);
      }
    };
    

so any use of eval is at risk of being missed in cases like that.

Member

loganfsmyth commented Nov 29, 2017

Yeah, the two main cases where we rename are

  • Modules, rewriting variables to property accesses.
  • Variable renames for function naming:
    var Map = window.Map;
    
    var obj = {
      Map(){
       	console.log(Map);
      },
    };
    
    becomes
    var _Map = window.Map;
    var obj = {
      Map: function Map() {
        console.log(_Map);
      }
    };
    

so any use of eval is at risk of being missed in cases like that.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Nov 29, 2017

Yup.

Node JS development in 2017: we don't have dependency hell, we have toolchain hell instead!

😄 no offense of course, babel and flow-runtime are both awesome projects, but getting all these layers of complexity to work easily is inherently difficult...

jedwards1211 commented Nov 29, 2017

Yup.

Node JS development in 2017: we don't have dependency hell, we have toolchain hell instead!

😄 no offense of course, babel and flow-runtime are both awesome projects, but getting all these layers of complexity to work easily is inherently difficult...

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Nov 29, 2017

Member

We definitely feel that pain too, no worries.

Member

loganfsmyth commented Nov 29, 2017

We definitely feel that pain too, no worries.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Aug 12, 2018

Member

Seems like this was resolved to the extent that it could be on our side.

Member

loganfsmyth commented Aug 12, 2018

Seems like this was resolved to the extent that it could be on our side.

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