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

Using arrays defined in vm not recognized by Blaze #each directive #8160

Closed
brucejo75 opened this issue Dec 15, 2016 · 17 comments
Closed

Using arrays defined in vm not recognized by Blaze #each directive #8160

brucejo75 opened this issue Dec 15, 2016 · 17 comments

Comments

@brucejo75
Copy link
Contributor

@brucejo75 brucejo75 commented Dec 15, 2016

I am creating arrays in a VM. Then I am using these arrays in my Blaze template. Blaze does not recognize that these are valid arrays.

Environment

meteor v1.4.2.3

Explanation

Blaze depends on the Meteor observe_sequence package. And the observe_sequence package uses instanceof Array to determine if the sequence is an array. instanceof Array does not work across windows, because there are 2 instances of Array one in each window.

There is a writeup here.

Fix

change these 2 lines to use Array.isArray(seq) instead of depending on instanceof Array.
observe_sequence.js#L97
observe_sequence.js#L129

@stubailo
Copy link
Contributor

@stubailo stubailo commented Dec 15, 2016

Would be great to see a PR for this - looks like a simple fix!

@mitar
Copy link
Collaborator

@mitar mitar commented Dec 16, 2016

I think we should just use _.isArray.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Dec 16, 2016

Isn't Array.isArray just a standard version of the exact same thing?

@mitar
Copy link
Collaborator

@mitar mitar commented Dec 16, 2016

Yes, and Underscore will use it if it is available, no?

@mitar
Copy link
Collaborator

@mitar mitar commented Dec 16, 2016

BTW, what is VM?

@stubailo
Copy link
Contributor

@stubailo stubailo commented Dec 16, 2016

Yeah I guess avoiding underscore dependencies is a lost cause at this point.

@mitar
Copy link
Collaborator

@mitar mitar commented Dec 16, 2016

I mean, we should replace underscore with lodash, but otherwise it is a good idea to have a layer of abstraction in between.

@brucejo75
Copy link
Contributor Author

@brucejo75 brucejo75 commented Dec 16, 2016

I am using vm.browserify to do my evals to keep the evals safe from global corruption. They refer to running it in a vm (aka virtual machine), but I think it is just really another window.

@abernix
Copy link
Member

@abernix abernix commented Dec 16, 2016

@brucejo75 Would there be anything stopping you from making a PR for this? In other words, is there anything I/Meteor/we can do to help you submit your first PR?

@brucejo75
Copy link
Contributor Author

@brucejo75 brucejo75 commented Dec 16, 2016

Thanks @abernix. I will take a shot at it. Never submitted a PR to Meteor. I will carve some time today.

@brucejo75
Copy link
Contributor Author

@brucejo75 brucejo75 commented Dec 16, 2016

@abernix, I may need some help. Here is what I did:

  1. forked meteor
  2. made code change in observe-sequence.js
  3. ran .\meteor test-packages
  4. pointed my browser here: http://localhost:3000/
  5. all observe-sequence tests pass.
  6. verified my error case is fixed.

1335 tests of 1390 passed. Most errors are this:

- exception - message Invalid array buffer length
RangeError: Invalid array buffer length
    at new ArrayBuffer (native)
    at C_MINISAT (packages\logic-solver.js:22:20101)
    at new MiniSat (packages\logic-solver.js:93:21)
    at new Logic.Solver (packages\logic-solver.js:620:19)
    at packages\constraint-solver.js:1575:26
    at Function.CS.DummyProfile.time (packages\constraint-solver.js:2322:10)
    at [object Object].CS.Solver._getAnswer (packages\constraint-solver.js:1574:11)
    at packages\constraint-solver.js:1561:17
    at Function.CS.DummyProfile.time (packages\constraint-solver.js:2322:10)
    at [object Object].CS.Solver.getAnswer (packages\constraint-solver.js:1560:23)

I took a look at C_MINISAT. single line of > 1000 characters. :-). But it looks like it has nothing to do with Blaze.

Next steps? Should I just submit the PR?

@mitar
Copy link
Collaborator

@mitar mitar commented Dec 17, 2016

Interesting. Didn't know about vm in the browser.

I think you should not worry about those other tests failing. So yes, create a PR.

@brucejo75
Copy link
Contributor Author

@brucejo75 brucejo75 commented Dec 17, 2016

OK, I submitted a pull request for this issue. Fairly benign.

I did a quick search of all instances of instanceof Array. There are quite a few argument checks for minimongo arguments that use instanceof, which could trip this issue again.

Mind if I take a look at this across Meteor? Should I create separate bug?

@mitar
Copy link
Collaborator

@mitar mitar commented Dec 19, 2016

I would change only things where you notice a problem.

@abernix
Copy link
Member

@abernix abernix commented Dec 19, 2016

Thanks for submitting that, @brucejo75. The test failures are likely because running the test command you're using would be running all tests across all packages, some of which are known to fail (and excluded explicitly). If you're ever developing on a particular package within Meteor you can run just that subset of the tests by passing the package name to test-packages. (e.g. .\meteor test-packages observe-sequence. Alternatively, the failures might be Windows-related as I'm not sure that entire suite is run on a Windows box.

If you can reproduce a problem with other instances, I think PRs for those are appropriate there as well but as @mitar says, only if you notice a problem. Feel free to open separate issues if you feel it's worth separate discussions but I think a sufficient explanation in the PR body referencing this issue might be okay if there are a lot of cases.

benjamn added a commit that referenced this issue Dec 19, 2016
fix for #8160
@mitar
Copy link
Collaborator

@mitar mitar commented Dec 29, 2016

#8166 has been merged in.

@mitar mitar closed this Dec 29, 2016
@brucejo75
Copy link
Contributor Author

@brucejo75 brucejo75 commented Feb 19, 2017

This fix has an issue. It does not work for sub-classed Array objects. See #8166 (comment).

I will work on another PR.

abernix added a commit that referenced this issue Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.