pbastowski / angular-meteor-babel

Babel 5 ES6 transpiler for angular-meteor (includes ng-annotate)
11 stars 4 forks source link

browser-polyfill breaks check method if not added before for phantom and older chromes #12

Closed mxab closed 8 years ago

mxab commented 8 years ago

Hi, the rendering with phantomjs for our webapp stopped working because the find method

myCollection.find({},{limit:5});

started to throw an error

Uncaught Error: Match error: Failed Match.OneOf or Match.Optional validation

after some debugging I noticed that the check method checks if the limit value is a number by comparing the given pattern "Number" its available types https://github.com/meteor/meteor/blob/devel/packages/check/match.js#L151

the Number function in typeofChecks is the native function but the pattern value given for checking is a Number function that seems to be defined in browser-polyfill and therefore fails the === test

a workarround seems to be adding the pbastowski:angular-babel as first package but I think it is very strange the the Number function is kind of overwritten by the polyfill

it can be seen in the "older" chromium browsers as well for example in Chromium 40.0.2214.0

pbastowski commented 8 years ago

@mxab What version of angular-babel are you using?

Also, you said "started to throw an error". When did it start doing that and what changes were made, if you can remember?

mxab commented 8 years ago

I didn't add any explicit version, it just happens if you use meteor's angular package

see this example: https://github.com/mxab/check-babel-problem

if you try to render it via phantomjs you will get an error Uncaught Error: Match error: Failed Match.OneOf or Match.Optional validation

if I add your package at the beginning of the packages file it will work again

pbastowski commented 8 years ago

Thanks for the repo. So, it's the latest 1.0.7, as listed in the .meteor/versions file.

It might be a silly question, but how do I render it under phantomjs?

mxab commented 8 years ago

run this script with phantom:

var page = require('webpage').create();
var url = 'http://localhost:3000/';

page.open(url, function (status) {
  phantom.exit();
});

if you want to see it in a browser, I recommend an older chromium https://www.chromium.org/getting-involved/download-chromium

I think I downloaded this one to debug the issue https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Mac/303344/

mxab commented 8 years ago

according to the chrome debugger the Number function comes from here https://github.com/pbastowski/angular-meteor-babel/blob/meteor1.2/lib/browser-polyfill.js#L2130

mxab commented 8 years ago

how do you generate the browser-polyfill.js? The overwritting happens here: https://github.com/pbastowski/angular-meteor-babel/blob/meteor1.2/lib/browser-polyfill.js#L2152

Is there any reason why Number is polyfilled?

pbastowski commented 8 years ago

I get the polyfill from babel-core node_modules/babel-core/browser-polyfill.js.

I would say there is no reason to polyfill unless it doesn't already exist. So, a good question is why is it being poly-filled without checking first for it's existence?

When I add a check for the existence of Number then the error is gone.

2102  if (typeof Number !== 'undefined') return;

I can add the above check to the polyfill's code and then we'd need to do some serious testing.

zloirock commented 8 years ago

https://github.com/zloirock/core-js/issues/38#issuecomment-76114855

pbastowski commented 8 years ago

Thanks @zloirock for the link.

So, I'd say option 1 is still the easiest solution (workaround) to implement, meaning, to move pbastowski:angular-babel to near the top of the packages file, as suggested by @mxab . Right?

zloirock commented 8 years ago

I don't use meteor. If you can require core-js / babel-polyfill before match - sure, it's the best solution. AFAIK most meteor packages use the third way - custom build or usage only required modules by CommonJS API.

pbastowski commented 8 years ago

@zloirock @mxab thanks for your help on this issue.

Since we have an easy workaround (adding pbastowski:angular-babel as first package) I plan to leave this issue as-is for the moment. Once I have upgraded the package to Babel 6.x, I can revisit the issue, if it still exists.

Closing for now, but, please feel free to re-open if required.

pbastowski commented 8 years ago

@mxab I have published pbastowski:angular-babel@1.0.9. Other users appeared to have similar problems to yours, so, I amended browser-polyfill.js to only polyfill Number if needed.

Let me know if this update fixes the problem for you.

Thanks.