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

Is that Array.prototype.concat will be removed during importing polyfilll? #1034

Closed
aleen42 opened this issue Jan 5, 2022 · 15 comments
Closed

Is that Array.prototype.concat will be removed during importing polyfilll? #1034

aleen42 opened this issue Jan 5, 2022 · 15 comments

Comments

@aleen42
Copy link
Contributor

@aleen42 aleen42 commented Jan 5, 2022

Recently, I have found that if I have shimmed Function.prototype.bind at first:

var $A = Array.from = function (iterable) {
    if (!iterable) return [];
    if (iterable.toArray) {
        return iterable.toArray();
    } else {
        var results = [];
        for (var i = 0; i < iterable.length; i++)
            results.push(iterable[i]);
        return results;
    }
};

Function.prototype.bind = function () {
    var __method = this, args = $A(arguments), object = args.shift();
    return function () {
        return __method.apply(object, args.concat($A(arguments)));
    };
};

And then, when running in the process of importing core-js/shim, Array.prototype.concat has been removed, and an exception was thrown when core-js tries to call shimmed Function.prototype.bind.

I just want to confirm whether it is due to the commit (277cb33), which wants to protect some built-in methods?

@aleen42 aleen42 changed the title Is that Array.prototype.concat will be removed during polyfilll? Is that Array.prototype.concat will be removed during importing polyfilll? Jan 5, 2022
@zloirock
Copy link
Owner

@zloirock zloirock commented Jan 5, 2022

Is that Array.prototype.concat will be removed during importing polyfill?

Only in case if it should be polyfilled and immediately will be replaced to the proper method.

And then, when running in the process of importing core-js/shim, Array.prototype.concat has been removed, and an exception was thrown when core-js tries to call shimmed Function.prototype.bind.

I can't reproduce that. I see only some possible issues from your incomplete .bind implementation that does not work with constructors.

@zloirock
Copy link
Owner

@zloirock zloirock commented Jan 5, 2022

For example, your code, core-js import and core-js test case:

image

Issues only in your .bind since core-js does not detect such cases of broken in the userland environment, Array.from is replaced to the proper. Nothing related to .concat.

@aleen42
Copy link
Contributor Author

@aleen42 aleen42 commented Jan 5, 2022

In my case, I have caught an error thrown when accessing args.concat in my code. It seems weird, and I have removed the shim code before importing polyfill from core-js to avoid conflicts.

You can try to add a debugger point here to check whether args.concat is undefined:

Function.prototype.bind = function () {
    var __method = this, args = $A(arguments), object = args.shift();
    return function () {
+       if (!args.concat) throw new Error('Ooops');
        return __method.apply(object, args.concat($A(arguments)));
    };
};

@zloirock
Copy link
Owner

@zloirock zloirock commented Jan 5, 2022

Please, add a reproducible example. I can't reproduce it just with this code.

@aleen42
Copy link
Contributor Author

@aleen42 aleen42 commented Jan 6, 2022

@zloirock Denis, we can reproduce the problem via my commit (aleen42@937d464)

$ npm run test-pollution

> test-pollution
> karma start -f=pollution.js,packages/core-js-bundle/index.js,tests/bundles/tests.js

PhantomJS 2.1.1 (Mac OS 0.0.0) WARN: 'Possible Unhandled Promise Rejection:', TypeError: undefined is not a constructor (evaluating 'args.concat($A(arguments))')
http://localhost:9876/base/pollution.js?8927b7e12418c94e8f945840857d4b03b1afa951:16:46
Error@http://localhost:9876/base/packages/core-js-bundle/index.js?3fbec792f23e50e2ca19b5626f36c32439075414:2854:48
done@http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:2082:26
advanceTestQueue@http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:1985:11
advance@http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:1940:23
unblockAndAdvanceQueue@http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:4321:28
http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:1634:19
PhantomJS 2.1.1 (Mac OS 0.0.0) ERROR

Since Karma has caught the error silently, we can get the detailed error stack by adding try...catch to wrap the whole packages/core-js-bundle/index.js. Here is the detailed stack:

$ npm run test-pollution

> test-pollution
> karma start -f=pollution.js,packages/core-js-bundle/index.js,tests/bundles/tests.js

PhantomJS 2.1.1 (Mac OS 0.0.0) LOG: TypeError: undefined is not a constructor (evaluating 'args.concat($A(arguments))')
http://localhost:9876/base/pollution.js?8927b7e12418c94e8f945840857d4b03b1afa951:16:46
http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:1174:46
exports@http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:1100:18
defineProperty@http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:600:26
http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:1521:32
exports@http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:1658:35
exports@http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:881:13
http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:3530:2
__webpack_require__@http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:26:34
http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:117:20
__webpack_require__@http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:26:34
http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:90:37
http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:91:12
global code@http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:21483:15
PhantomJS 2.1.1 (Mac OS 0.0.0) WARN: 'Possible Unhandled Promise Rejection:', TypeError: undefined is not a constructor (evaluating 'args.concat($A(arguments))')
http://localhost:9876/base/pollution.js?8927b7e12418c94e8f945840857d4b03b1afa951:16:46
Error@http://localhost:9876/base/packages/core-js-bundle/index.js?24ba48fc928bc58b4ab582da317c09088845b4f6:2854:48
done@http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:2082:26
advanceTestQueue@http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:1985:11
advance@http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:1940:23
unblockAndAdvanceQueue@http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:4321:28
http://localhost:9876/base/node_modules/qunit/qunit/qunit.js?c9a269d8ad4db3a08f5a483c41b9bce84062fc4f:1634:19
PhantomJS 2.1.1 (Mac OS 0.0.0) ERROR

@aleen42
Copy link
Contributor Author

@aleen42 aleen42 commented Jan 6, 2022

I think core-js can try to remove all external shims at the start point to avoid such a problem, but it seems not necessary. It is just a suggestion. The main issue is can we avoid removing Array.prototype.concat?

@zloirock
Copy link
Owner

@zloirock zloirock commented Jan 6, 2022

@aleen42 it's not an external shim, since it's an incomplete polyfill that unconditionally replaces the proper feature, it's a case of a broken environment. Need to check, most likely this error happens between Array.prototype.concat removing and adding the proper polyfill. However, I'm not sure that it's something that should be fixed since it's a case of a broken environment.

@aleen42
Copy link
Contributor Author

@aleen42 aleen42 commented Jan 6, 2022

A possible direction is to confirm using native methods or shim methods of core-js rather than untrusted ones, during the procedure of removing Array.prototype.concat and replacing back.

@zloirock
Copy link
Owner

@zloirock zloirock commented Jan 6, 2022

Yes, I think I'll do something like that since such .bind was popular some time ago.

@aleen42
Copy link
Contributor Author

@aleen42 aleen42 commented Jan 6, 2022

👍🏼

@zloirock
Copy link
Owner

@zloirock zloirock commented Jan 6, 2022

Added a workaround.

Note: Most likely, this check will be removed in core-js@4 with ES3 support.

Thanks for the issue.

@aleen42
Copy link
Contributor Author

@aleen42 aleen42 commented Jan 6, 2022

@zloirock is it better to add a testing case for such a polluted environment? But it seems not proper to add polluted code into this repository.

@zloirock
Copy link
Owner

@zloirock zloirock commented Jan 6, 2022

Anyway, we can't detect all possible cases of broken in the userland environment. Now, we have fallbacks for cases with missed .bind, in case of removing ES3 support their removal is implied. Leaving such code will significantly increase the size of minimalistic bundles and it will significantly reduce pluses of removing ES3 support.

@aleen42
Copy link
Contributor Author

@aleen42 aleen42 commented Jan 6, 2022

OK, it is just a choice, and feel free to close it. #1035

@zloirock
Copy link
Owner

@zloirock zloirock commented Jan 6, 2022

My previous comment was a continuation of my comment what was before.

@zloirock is it better to add a testing case for such a polluted environment? But it seems not proper to add polluted code into this repository.
#1035

I'm not sure that makes sense - it's a too specific case, I'll think about it, thanks.

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

No branches or pull requests

2 participants