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

Replace old shim with current from mozilla #15

Closed
wants to merge 1 commit into from
Closed

Conversation

ff6347
Copy link
Member

@ff6347 ff6347 commented Oct 27, 2019

I'm working on setting up automated tests and the array every seems not to work right. Replaced it with the current one from MDN and voilà

@ff6347 ff6347 requested a review from EugenTepin October 27, 2019 18:49
@ff6347
Copy link
Member Author

ff6347 commented Oct 27, 2019

Same goes for Array.filter? They all break at

callback.__class__ !== "Function"

@ff6347
Copy link
Member Author

ff6347 commented Oct 28, 2019

I tested the old one in InDesign and it works.
Tested the new one and it works as well.

I'll be setting up a small testing utility we can use the write test we can run locally.

@lumenn
Copy link
Member

lumenn commented Oct 29, 2019

I tested the old one in InDesign and it works.
Tested the new one and it works as well.

I'll be setting up a small testing utility we can use the write test we can run locally.

Anything i could help with?

@ff6347 ff6347 mentioned this pull request Oct 30, 2019
@ff6347
Copy link
Member Author

ff6347 commented Oct 30, 2019

@lumenn See #17

We should not mix setting up the tests with PRs like this one. I saw that Array.every also was updated on MDN. So either writing tests or walking through the shims and seeing if they are still up to date would both be a great help.

@EugenTepin
Copy link
Contributor

@fabianmoronzirfas

I saw that Array.every also was updated on MDN.

It's been a long time, but it seems to me that the version of polyfills has not changed on MDN. I have changed some implementation details of polyfills, when I was working on it. __class__ is among this edits. This is internal property which shows the actual type of the object (only Extendscript). I think it is safe to revert this polyfill to its original state. I have some thoughts about that, I will share them later.

@ff6347
Copy link
Member Author

ff6347 commented Oct 30, 2019

I have changed some implementation details of polyfills, when I was working on it. class is among this edits.

Ah. okay. We need to decide if we keep it that way or if we make it more ECMA compatible. Please share your thoughts when you have time to

@EugenTepin
Copy link
Contributor

Ok, by initial design I plan to use this shims only in Extendscript engine, and I do not think that Adobe have changed script implementations in their products, so, please be sure that you launching this scripts in proper environment.
__class__ property was used, because it gets more precise value of object type. Please, see this link, but this is only one rare case that I have found where __class__ is better.
So, I think it is safe to replace my implementation with initial implementation from mdn. If you will decide to do that, please, also take into account __proto__ value.

@ff6347
Copy link
Member Author

ff6347 commented Nov 4, 2019

Hm. Sounds more complicated then just replacing one shim. I'll close this and add a new issue.

@ff6347 ff6347 closed this Nov 4, 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.

3 participants