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

JS::Object inherits BasicObject #541

Merged
merged 7 commits into from
Nov 1, 2024

Conversation

ledsun
Copy link
Contributor

@ledsun ledsun commented Oct 19, 2024

The original motivation was to make it easier to call the JavaScript WebSocket#send method.

To achieve this, the following three methods were considered.

  1. Undefine the Object#send method
  2. Override the Object#send method
  3. Stop inheriting from the Object class

I was attracted by the fact that it clearly defines the role of the JS::Object class as a class that wraps JavaScript objects. I adopted the third method.

You can see past challenges at #509.


# I don't know why, but I can't define the respond_to? method in refinements.
# I'm defining it here instead.
[:nil?, :is_a?, :raise, :respond_to?].each do |method|
Copy link
Member

Choose a reason for hiding this comment

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

Given that JS objects unlikely define methods suffixed with ?, it makes sense to me to define nil?, is_a?, and respond_to? for convenience and better compatibility with libraries without BasicObject support.
One question, where do we use raise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, it seems that there is no need to consider collisions of methods suffixed with ?.

We raise an exception when the method call fails.

raise NoMethodError,
"undefined method `#{sym}' for an instance of JS::Object"

require 'pp'
module JsObjectTestable
refine JS::Object do
[:object_id, :pretty_inspect].each do |method|
Copy link
Member

Choose a reason for hiding this comment

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

If those definitions are just for testing, it might be better to fix testing libraries in the long term. I just opened a test-unit fix for BasicObject support. test-unit/test-unit#262

But it's fine for now. Could you add FIXME comments mentioning the fix PR and potential future fix?

@ledsun ledsun marked this pull request as ready for review October 27, 2024 15:10
@kateinoigakukun kateinoigakukun merged commit 7d15e01 into ruby:main Nov 1, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants