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

alternative for object.create under IE8 #727 #728

Merged
merged 1 commit into from
Dec 17, 2019

Conversation

aleen42
Copy link
Contributor

@aleen42 aleen42 commented Dec 14, 2019

PR for #727

@aleen42
Copy link
Contributor Author

aleen42 commented Dec 14, 2019

@zloirock Why can't I see the log of building failed above TravisCI?

@zloirock
Copy link
Owner

I don't know. It's linting issues, you will have the same with local tests.

@aleen42
Copy link
Contributor Author

aleen42 commented Dec 14, 2019

OK, I'll fix the problem mentioned after lint

@aleen42 aleen42 force-pushed the object.create branch 2 times, most recently from 9370eff to d713e1f Compare December 14, 2019 13:45
@aleen42 aleen42 force-pushed the object.create branch 5 times, most recently from 588dfb8 to 59ace3a Compare December 14, 2019 14:49
Copy link
Contributor Author

@aleen42 aleen42 left a comment

Choose a reason for hiding this comment

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

I have tested activeXDocument under IE11, which simulates IE8, and found that if I declared it inside NullProtoObject method, this method would be released when AcitiveXObject was released, result in script error. So I have to put it into a more global scope.

@zloirock
Copy link
Owner

zloirock commented Dec 16, 2019

Did you run tests on real IE8? Seems sometimes they are fails.

(fails of JSON and names of .upsert-related methods tests in IE8 is OK)

@aleen42
Copy link
Contributor Author

aleen42 commented Dec 17, 2019

What detailed error have you caught? I have no real IE8 for karma to launch to test.

@zloirock
Copy link
Owner

zloirock commented Dec 17, 2019

You could use VMs. You don't need karma for tests. Run npm test for usual CI and bundling all required files - and you could just open tests/tests.html file in IE8.

Details - when I tested, for some unknown reason in ~50% of the page loading some built-ins were unavailable. I don't know - it could be just the network problems or it's the problem with this script - since it's IE and specific environment object. I hadn't enough time for exploring the problem. Little later I'll try to dig deeper.

@aleen42
Copy link
Contributor Author

aleen42 commented Dec 17, 2019

image

I have the same error on the master branch, and the error was thrown when running the following snippet:

var _ref = Reflect || {},
    ownKeys = _ref.ownKeys;

It may be an existed bug before (same after testing under @3.5.0)

@zloirock
Copy link
Owner

Interesting. I was able to reproduce it too but much less with 50% chance. However, I don't remember any changes that could affect IE8.

@zloirock zloirock merged commit 5e193bf into zloirock:master Dec 17, 2019
@aleen42
Copy link
Contributor Author

aleen42 commented Dec 18, 2019

I can reproduce with 100% chance, and I hope I can help you find out later.

@aleen42 aleen42 deleted the object.create branch December 18, 2019 01:06
@zloirock
Copy link
Owner

@aleen42 how?

@aleen42
Copy link
Contributor Author

aleen42 commented Dec 18, 2019

The same way, but caught Reflect is not defined any time

@zloirock
Copy link
Owner

zloirock commented Dec 18, 2019

I already can't reproduce this problem completely.

VirtualBox_8_18_12_2019_08_11_04

@aleen42
Copy link
Contributor Author

aleen42 commented Dec 18, 2019

OK, I'll check it later.

@aleen42
Copy link
Contributor Author

aleen42 commented Dec 18, 2019

What I need to test is pure.html rather than tests.html.

Real IE8:

image

Simulating IE8 under IE11:

image

@zloirock
Copy link
Owner

The same - no problems.

VirtualBox_8_18_12_2019_08_52_06

@aleen42
Copy link
Contributor Author

aleen42 commented Dec 18, 2019

👌

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

Successfully merging this pull request may close these issues.

2 participants