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

[1.4.2-rc.4] super does not work in some cases because __proto__ is not set #7956

Closed
macrozone opened this issue Oct 25, 2016 · 23 comments
Closed
Assignees

Comments

@macrozone
Copy link
Contributor

Meteor 1.4.2-rc.4 seems to break some npm packages: vazco/uniforms#117

I could not fully track this down, but what I found out is, that the meteor-version of babel-runtime does not set proto / Object.setPrototypeOf

Compare this:
meteor version: https://github.com/meteor/meteor/blob/devel/packages/babel-runtime/babel-runtime.js#L81

with this:
babel version: https://github.com/babel/babel/blob/master/packages/babel-helpers/src/helpers.js#L378

super() is transformed to this:

var _this = (0, _possibleConstructorReturn3['default'])(this, (_class.__proto__ || Object.getPrototypeOf(_class)).call(this, props));

When running under meteor 1.4.2, _class.__proto__ is just an empty anonymous function, whereas under meteor 1.4.1.x _class.__proto__ is set to a constructor-function

It might be also related to the fact, that this particular package uses static properties. I can't imagine, that inheritance just breaks without noticing.

Anyone an idea, what's going on here?

@benjamn
Copy link
Contributor

benjamn commented Oct 25, 2016

I agree this is a bug. Fortunately we can fix it with an update to babel-runtime without doing a whole Meteor release.

@macrozone
Copy link
Contributor Author

As 1.4.2 is now released, how can i update babel-runtime separatly?

@maartenbusstra
Copy link

Is this why my 2x subclassing of Mongo.Collection fails? It says insert or remove are undefined functions.

@neurosoup
Copy link

Same for me, I am subclassing Mongo.Collection and have error 'insert undefined' since 1.4.2 release. Is the issue is linked ?

@benjamn
Copy link
Contributor

benjamn commented Oct 26, 2016

Here's the original explanation of why we chose not to set __proto__. In short, it wouldn't work in Internet Explorer <10, and any code you wrote that relied on that feature would appear to work in other browsers, but fail in older IEs, where you might not be actively testing.

The good news is that Babel now attempts to work around the limitations of older IEs by using _class.__proto__ || Object.getPrototypeOf(_class) instead of Object.getPrototypeOf(_class), thanks to this commit.

@benjamn
Copy link
Contributor

benjamn commented Oct 26, 2016

I think I know how to fix this, but I'm having trouble reproducing it. Can anyone provide a small example app that has this problem?

@maartenbusstra
Copy link

maartenbusstra commented Oct 26, 2016

I tried to, but I can't get it to not work either in a new project.

In my current project, however, this code:

import { Mongo } from 'meteor/mongo';

export default class Collection extends Mongo.Collection {
  constructor({ name, schema, helpers } = {}) {
    super(name);
    if (schema) this.attachSchema(schema);
    if (helpers) this.helpers(helpers);
  }
}

console.log(Collection);
console.log(new Collection({ name: 'test' }));

yields:

{ [Function: Collection]
I20161027-01:08:55.358(2)?   _publishCursor: [Function],
I20161027-01:08:55.358(2)?   _rewriteSelector: [Function],
I20161027-01:08:55.358(2)?   Cursor: [Function],
I20161027-01:08:55.358(2)?   ObjectID: [Function] }
I20161027-01:08:55.359(2)? Collection {}

package.json deps:

"dependencies": {
    "apollo-client": "^0.4.19",
    "apollo-server": "^0.3.2",
    "body-parser": "^1.15.2",
    "casual": "^1.5.5",
    "classnames": "^2.2.5",
    "cloudinary": "^1.4.3",
    "eslint-config-airbnb": "^12.0.0",
    "express": "^4.14.0",
    "flickity-fix": "^1.1.2",
    "form-data": "^2.1.1",
    "graphql": "^0.7.0",
    "graphql-tools": "^0.7.2",
    "graphql-typings": "0.0.1-beta-2",
    "key-mirror": "^1.0.1",
    "meteor-node-stubs": "~0.2.0",
    "node-fetch": "^1.6.3",
    "normalize.css": "^4.2.0",
    "prerender-node": "^2.1.0",
    "radium": "^0.18.1",
    "react": "^15.3.2",
    "react-apollo": "^0.5.6",
    "react-dom": "^15.3.2",
    "react-helmet": "^3.1.0",
    "react-motion": "^0.4.5",
    "react-redux": "^4.4.5",
    "react-router": "^2.8.1",
    "react-router-transition": "0.0.6",
    "react-scroll": "^1.4.0",
    "redux": "^3.6.0",
    "tinycolor2": "^1.4.1"
  },
  "devDependencies": {
    "babel-eslint": "^7.0.0",
    "babel-plugin-inline-import": "^2.0.4",
    "babel-plugin-syntax-async-functions": "^6.13.0",
    "babel-plugin-transform-async-to-generator": "^6.16.0",
    "babel-preset-es2015": "^6.14.0",
    "babel-preset-react": "^6.11.1",
    "babel-preset-stage-2": "^6.13.0",
    "eslint": "^3.5.0",
    "eslint-config-airbnb": "^11.1.0",
    "eslint-import-resolver-meteor": "^0.3.3",
    "eslint-plugin-import": "^1.16.0",
    "eslint-plugin-jsx-a11y": "^2.2.2",
    "eslint-plugin-meteor": "^4.0.0",
    "eslint-plugin-react": "^6.3.0"
  }

.meteor/packages:

[email protected]
[email protected]
[email protected]
[email protected]
[email protected]

[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
static-html
apollo
dburles:collection-helpers
aldeed:simple-schema
meteorhacks:picker
aldeed:collection2
jabbslad:basic-auth
[email protected]
meteorhacks:kadira
[email protected]

.meteor/versions:

[email protected]
aldeed:[email protected]
aldeed:[email protected]
aldeed:[email protected]
aldeed:[email protected]
aldeed:[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
dburles:[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
jabbslad:[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
mdg:[email protected]
[email protected]
[email protected]
meteorhacks:[email protected]
meteorhacks:[email protected]
meteorhacks:[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]_2
[email protected]
[email protected]
[email protected]
raix:[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
tmeasday:[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]

@benjamn benjamn added this to the Release 1.4.2.1 milestone Oct 26, 2016
@maartenbusstra
Copy link

FYI: I also added my packages file.

@macrozone
Copy link
Contributor Author

@benjamn: so why was it working in previous versions of meteor? I never checked on IE versions below 10, was it just not working there?

I am also not sure, if it is a good decision to make a regression for all browsers, because it does not work on a IE < 10...

anyway, I hope 1.4.2.1 will land soon, that we can profit from the awesome performance tweaks :-)

I try to setup a reproduction...

@macrozone
Copy link
Contributor Author

ok @benjamn , here is a reproduction based on uniforms's demo-project.

I added a custom form-theme that uses static classes along the themes installed from npm. (mine is a uncompiled copy from uniforms-unstyled)

https://github.com/macrozone/meteor-1.4.2-static-inheritance-reproduction

clone, npm install and start it. It's running meteor 1.4.1.

Then on the page, you can select the form-themes. Every theme including my local "custom"-theme should work.

Then update to meteor 1.4.2.

~~now, the custom-form will not work anymore. (will throw an error in the console). ~~

On my first try, only my custom-form did not work anymore. But now, as i am writing this issues, all theme do not work anymore under 1.4.2 due to the same problem.

hope this helps.

@benjamn
Copy link
Contributor

benjamn commented Oct 27, 2016

@macrozone It worked in previous versions of Meteor because we just copied static properties, which worked equally well in all browsers. What changed was a newer version of Babel assuming that __proto__ had been set.

@benjamn
Copy link
Contributor

benjamn commented Oct 27, 2016

Ok, after digging into that reproduction (thanks @macrozone!), it looks like the source of the problem is code in node_modules that wasn't compiled by Meteor, such as AutoForm.js. I think this code worked before 1.4.2 because it was able to use node_modules/babel-runtime/helpers/inherits.js, whereas 1.4.2 ignores the babel-runtime helpers that are provided by babel-runtime.js, and the version of inherits that it uses doesn't set __proto__.

Short-term, making sure babel-runtime.js matches the behavior of the babel-runtime npm package regarding __proto__ should fix the immediate problem.

Longer term, we need to evaluate whether implementing Babel helpers in a Meteor specific way still has its original values: minimizing the use of unnecessary runtime libraries, and supporting older browsers. If babel-runtime meets those requirements now (or comes close enough), then the Meteor babel-runtime package should probably go away and babel-runtime should become part of every app's package.json.

@benjamn
Copy link
Contributor

benjamn commented Oct 28, 2016

This should be fixed if you run meteor update --release 1.4.2.1-beta.0 in your app directory.

@macrozone
Copy link
Contributor Author

ok, i updated to 1.4.2.1-beta.0, but the problem still exists. Do i need to meteor reset, wipe node_modules or similar?

@maartenbusstra
Copy link

Might be your build cache.
On Mon, 31 Oct 2016 at 09:49, Marco Wettstein [email protected]
wrote:

ok, i updated to 1.4.2.1-beta.0, but the problem still exists. Do i need
to meteor reset, wipe node_modules or similar?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7956 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFRpBsED25nPnOmZy5AaShhJZCnbCupbks5q5auygaJpZM4Kf4Dd
.

@sakulstra
Copy link
Contributor

sakulstra commented Oct 31, 2016

I think this is somehow related - also got the error after upgrading to 1.4.2

What I did now is:
meteor update --release 1.4.2.1-beta.0
meteor reset
rm node_modules -r && meteor npm install
meteor npm run start

I still get the same uniforms error
Warning: Failed childContext type: Required child contextuniforms.modelwas not specified inAutoValidatedQuickUnstyledForm.

@radekmie
Copy link
Collaborator

@sakulstra
Yes, it's related, because Auto overrides getChildContextModel, so it pass the model from the state, but it's not set, because super in constructor of Unstyled is noop.

@benjamn
Unluckily I have to agree with @macrozone - even after clearing node_modules and doing meteor reset, it's still not working in 1.4.2.1-beta.0.

@mimamuh
Copy link

mimamuh commented Oct 31, 2016

Me too, doesn't work after updating to 1.4.2.1-beta.0. Can't run a classes as the constructor won't be executed.

@maartenbusstra
Copy link

Mine worked after deleting node_modules and .meteor/local.
On Mon, 31 Oct 2016 at 18:06, Michael Neumair [email protected]
wrote:

Me too, doesn't work after updating to 1.4.2.1-beta.0. Can't run a
classes as the constructor won't be executed.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7956 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFRpBkKIIC8k3gipGM5dkyCP3YCi_jSTks5q5iAWgaJpZM4Kf4Dd
.

benjamn pushed a commit that referenced this issue Oct 31, 2016
In light of issues like #7956, I would very much like for app developers
to be responsible for providing node_modules/babel-runtime, and for the
Meteor babel-runtime package to stop attempting to implement a subset of
the helpers.
benjamn pushed a commit that referenced this issue Oct 31, 2016
Though this may seem like a significant change, this package will still
work for older apps as long as the developer follows the instructions
about installing the babel-runtime npm package.

Helps with #7956.
@benjamn benjamn reopened this Oct 31, 2016
@benjamn
Copy link
Contributor

benjamn commented Oct 31, 2016

My commit 9dfcc38 might be enough to fix this issue in combination with reinstalling node_modules and/or removing .meteor/local, but I think we can do better than that, so I'm reopening this.

@maartenbusstra
Copy link

Perhaps doing a build-cache clear post release update would suffice?

@macrozone
Copy link
Contributor Author

@benjamn clearing .meteor/local did not help or instlaling node_modules.

instead, i had to bump version in .meteor/versions of babel-runtime to [email protected]

benjamn pushed a commit that referenced this issue Nov 2, 2016
In light of issues like #7956, I would very much like for app developers
to be responsible for providing node_modules/babel-runtime, and for the
Meteor babel-runtime package to stop attempting to implement a subset of
the helpers.
benjamn pushed a commit that referenced this issue Nov 2, 2016
Though this may seem like a significant change, this package will still
work for older apps as long as the developer follows the instructions
about installing the babel-runtime npm package.

Helps with #7956.
@benjamn
Copy link
Contributor

benjamn commented Nov 3, 2016

Meteor 1.4.2.1-beta.1 contains some noteworthy changes to the way babel-runtime works, thanks to #7995. Please try the reproduction steps again after running meteor update --release 1.4.2.1-beta.1 and meteor npm install --save babel-runtime.

Closing now because the problem is fixed for me, but please reopen if you discover otherwise.

@benjamn benjamn closed this as completed Nov 3, 2016
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

No branches or pull requests

7 participants