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

Fix #461: Handle member functions in from js import ... #463

Closed
wants to merge 1 commit into from
Closed

Fix #461: Handle member functions in from js import ... #463

wants to merge 1 commit into from

Conversation

mdboom
Copy link
Collaborator

@mdboom mdboom commented Jun 12, 2019

No description provided.

@mdboom mdboom requested review from madhur-tandon and rth June 12, 2019 13:52
@rth
Copy link
Member

rth commented Jun 18, 2019

It looks like this makes test_jsproxy_kwargs test fail...

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

So the test_install_simple now failed twice in a row on Firefox (I restarted CI), with

> selenium_standalone.run("micropip.install('snowballstemmer')")
...
selenium.common.exceptions.WebDriverException: Message: NetworkError: A network error occurred.

Could it have been affected?

@mdboom
Copy link
Collaborator Author

mdboom commented Jun 20, 2019

Yeah -- I think this has broken the XHR API being used by micropip... Something about detecting the member function vs. free function is still not quite correct...

@hoodmane
Copy link
Member

@dalcde @rth I think this should be closed, it's obsoleted by #972.

@hoodmane
Copy link
Member

The one thing that looks interesting here is:

EM_JS(int, hiwire_is_member_function, (int idobj), {
  var jsobj = Module.hiwire_get_value(idobj);
  // clang-format off
  return (
      typeof jsobj === "function" &&
      jsobj.prototype === undefined &&
      jsobj.constructor !== undefined
  );
  // clang-format on
});

but I don't know how that works.

@rth
Copy link
Member

rth commented Dec 31, 2020

I think this should be closed, it's obsoleted by #972.

You can indicate "Closes #972" in that PR description to auto-close this on merge.

@hoodmane
Copy link
Member

hoodmane commented Jan 16, 2021

@rth @dalcde Okay, #1126 was just merged which resolves #461 so I think this can be closed.

@hoodmane hoodmane closed this Mar 5, 2021
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