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

Add Object.getOwnPropertyNames tests #368

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Xotic750
Copy link
Contributor

I'm experiencing some issues since loading es6-shim on my projects, I believe the issue is with Object.getOwnPropertyNames but I'm not sure if it is an ES5 or an ES6 issue yet, or maybe both. I'm still unable to get my fork building on Travis CI and need to PR it to begin figuring things out. It maybe that this ends up being closed here and opened over in es6-shim.

@Xotic750
Copy link
Contributor Author

Some of the problems show here, but are they fixable, I don't know/not sure yet?
I was already taking care of these in my app by filtering the results, but if fixable that would be great.
I guess I need to add some tests to es6-shim for the others that have shown up.

@Xotic750 Xotic750 changed the title Add Object#getOwnPropertyNames tests Add Object.getOwnPropertyNames tests Dec 13, 2015
});

itHasUint8Array('works on Uint8Array', function () {
expect(Object.getOwnPropertyNames(new Uint8Array(4))).toEqual(['0', '1', '2', '3']);
Copy link
Member

Choose a reason for hiding this comment

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

typed array, arraybuffer, and dataview tests don't belong in es6-shim right now, since it doesn't address either one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have a couple of other tests to add to this, but I'm not necessarily expecting any fixes. All of these, in this first push, I am already dealing with in app, but if any fixes come of them then it will be great. One of the problems that shows with es6-shim loaded is constructor shows up on Map and Set. I'll add the rest soon. My fork of es6-shim works on Travis without having to PR and I can just link to the results so that you can see.

Copy link
Member

Choose a reason for hiding this comment

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

That'd be a Map and Set test, not a getOwnPropertyNames test :-) thanks for reporting that, I'll see if I can fix it.

@Xotic750 Xotic750 force-pushed the getOwnPropertyNames branch 6 times, most recently from 42d8b4c to ce8db2e Compare December 13, 2015 14:42
@Xotic750
Copy link
Contributor Author

This shows the majority of differences that I am seeing with just es5-shim loaded, and then there are others that I see with es6-shim also loaded. Feel free to comment as usual, there's always room for improvement. ;)

@ljharb
Copy link
Member

ljharb commented Dec 13, 2015

Since this is the es5-shim, no code dealing with ES6 constructs, like Typed Arrays, Maps, Sets, DataViews, etc, belongs in this repo.

Please file es6-shim issues (including ES5 features interacting with ES6 features) on es6-shim, and I'll be happy to fix them.

@ljharb
Copy link
Member

ljharb commented Dec 13, 2015

For all these test cases, can you include a comment indicating where it fails? Object.getOwnPropertyNames isn't shimmable, so I'd only want to address things that fail in at least one engine.

@Xotic750
Copy link
Contributor Author

I'll split out tests and include some comments over the next week. I tried to exclude any tests where it is shammed, by limiting them to supportsDescriptors.

@ljharb
Copy link
Member

ljharb commented Jan 7, 2020

@Xotic750 can you check the "allow edits" box on the RHS of the PR? it'd also be great if you wanted to rebase this so we can get it in :-)

@ljharb
Copy link
Member

ljharb commented Mar 22, 2020

ping @Xotic750

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