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

Complete fix for #8160, handle subclassed objects. Added test #8401

Closed
wants to merge 0 commits into from
Closed

Complete fix for #8160, handle subclassed objects. Added test #8401

wants to merge 0 commits into from

Conversation

brucejo75
Copy link
Contributor

@ManuelDeLeon, discovered a flaw with PR #8166 which is a fix for #8160. It did not work for sub-classes of an Array.

Changed all instances of checking the array to be seq instanceof Array || _.isArray(seq).

Passes all observe-sequence tests. Added a test that verifies this works for subclassed Arrays.

@brucejo75
Copy link
Contributor Author

Also, I want to create another test that will verify it works for arrays created in a vm because that was the reason for the fix in the first place.

To add this test I need to add a vm-browserify as a node_module for observer_sequence_tests.js.

If someone can point me in the direction to do that, I can get it done. @abernix? @mitar?

return ArraySubclass;

})(Array);

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this sub-classing of a built-in is difficult, and it'll be ideal to remove eventually. I'm not sure if it would be sufficient for this test, but the following might be more succinct and work?

function ArraySubclass() {
  const Fp = Function.prototype;
  const inst = new (Fp.bind.apply(Array, [null].concat(Array.from(arguments))));
  inst.__proto__ = ArraySubclass.prototype;
  return inst;
}
ArraySubclass.prototype = Object.create(Array.prototype);

and then I believe the code below would be fine:

var seq = new ArraySubclass(1, 1, 2);

Copy link
Contributor Author

@brucejo75 brucejo75 Feb 21, 2017

Choose a reason for hiding this comment

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

Thanks @abernix,

Much simpler, unfortunately, the class you provided returns true to _.isArray(seq). We need:

var seq = new ArraySubclass(1, 1, 2);

seq instanceof ArraySubclass === true
seq instanceof Array === true
_.isArray(seq) === false

I think @ManuelDeLeon took his code from the code generated by the Coffeescript extends. So it might be good to stick with it? As this will likely crop up for Coffeescript users.

Also, any thoughts on being able to test the vm issue? Can I add vm-browserify to the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried using vm directly in the tests.js? meteor-node-stubs provides vm-browserify itself.

Copy link
Contributor Author

@brucejo75 brucejo75 Feb 22, 2017

Choose a reason for hiding this comment

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

Thanks @abernix, I just tried it.

vm is not a global
I cannot require('vm') - barfs on require
I cannot required('vm') - bars on required
I cannot Npm.required('vm') - barfs on Npm
I cannot import vm from 'vm'; - barfs on import
I cannot import {vm} from 'vm'; - barfs on import

Do you or @benjamn know how a test file can load a module? I do not see any require-like statements in the test files unless they are in node_modules installed under the .npm directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abernix, @benjamn Any help?

Copy link
Contributor

Choose a reason for hiding this comment

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

After further consideration, my recommendation wouldn't have worked since meteor-node-stubs aren't available from the tool tests, though you should be able to write a similar test in your own app and have it work – assuming you have meteor-node-stubs installed.

@@ -126,7 +126,7 @@ ObserveSequence = {
fetch: function (seq) {
if (!seq) {
return [];
} else if (_.isArray(seq)) {
} else if (seq instanceof Array || _.isArray(seq)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you declare a helper function in this file called isArray that has this implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, will implement:

function isArray(arr) { return seq instanceof Array || _.isArray(arr)}

Code changed to:

 } else if (isArray(seq)) {

@brucejo75
Copy link
Contributor Author

  • updated with @benjamn's change request.
  • updated tests to include sub classed Arrays

I would still like to add a test for an array created in a vm. But I do not know how to include the vm-browserify package into the tests.

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Despite the inability to add vm tests, from what I know about the way Array behaves in that environment, and further proven by your own testing, I'll sign off on this because the test you added should accurately catch any regression with this in the future.

I realize that's not the best answer since it fails to implement a test for the exact circumstance that caused this originally but instead tests for it "in spirit", but making the test work otherwise is currently not simple and this change is obviously important.

@brucejo75
Copy link
Contributor Author

brucejo75 commented Feb 24, 2017

Unfortunate. All of the tests pass if _.isArray(arr) removed from the isArray function. It is vm that needs _.isArray(arr).

@abernix
Copy link
Contributor

abernix commented Feb 24, 2017

Just as a sanity check for what might be a change that happens in the near future: does Array.isArray work for catching the vm Array.prototype as well, in the same way as Underscore's _.isArray does right now?

(I ask because Lodash is coming to Meteor and _.isArray is a direct mapping to Array.isArray in Lodash)

@brucejo75
Copy link
Contributor Author

Array.isArray works fine. Also, I already checked lodash's implementation against vm created arrays. So no issues with that.

@brucejo75
Copy link
Contributor Author

brucejo75 commented Feb 25, 2017

I was able to trim down the vm-browserify code enough that it made a reasonable function to include in the test file. Now I have tests for a vm generated array. To Summarize:

  • Made simple code change to test arrays via seq instanceof Array || _.isArray(seq). Implemented as function per @benjamn request.
  • Verified that this will work if _.isArray(seq) is switched to lodash, which is Array.isArray(seq)
  • Added a test for subclassed objects, reduced the ArraySubClass code size a little bit.
  • Added a test for "vm generated arrays"

I ran the tests with all combinations of seq instanceof Array and _.isArray(seq) and verified the tests fail as expected.

Should be complete.

@abernix abernix added this to the Release 1.4.3.x milestone Mar 1, 2017
@abernix abernix closed this Mar 1, 2017
@abernix
Copy link
Contributor

abernix commented Mar 1, 2017

Thanks for taking care of this, @brucejo75! Looks good.

I made one more commit (eb45830) to fix some formatting before merging this however you didn't have a branch on your account aside from your devel branch so when I pushed to it, this PR was disassociated with it. In the future, make sure you do development work on a branch within your project and not directly on the devel branch so merging works without us having to push to your main branch.

Alternatively, I guess you could disable "Allow contributors to make changes" when opening the PR, and then we'll just do it on our own branch instead of yours. Typically though, if you have branched it wouldn't be a problem.

Thanks again!

@brucejo75
Copy link
Contributor Author

Thanks for the tip! I was unaware of it. I will try to improve next time. Thanks!

@abernix
Copy link
Contributor

abernix commented Mar 9, 2017

This should be fixed in Meteor 1.4.3.2. You can try the latest 1.4.3.2 beta and help confirm by running:

meteor update --release 1.4.3.2-beta.0

Please report back if you encounter any problems, and thanks for taking care of this!

@abernix abernix modified the milestones: Release 1.4.3.x, Release 1.4.3.2 Mar 9, 2017
@brucejo75
Copy link
Contributor Author

brucejo75 commented Mar 9, 2017

Verified works for my test: ArraySubclassed & VM test. @ManuelDeLeon you might want to check it for your case too.

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.

None yet

3 participants