Skip to content

[WIP] Bridge improvements#2586

Draft
janbiedermann wants to merge 8 commits intoopal:masterfrom
janbiedermann:build_bridge_with_ropes
Draft

[WIP] Bridge improvements#2586
janbiedermann wants to merge 8 commits intoopal:masterfrom
janbiedermann:build_bridge_with_ropes

Conversation

@janbiedermann
Copy link
Copy Markdown
Member

No description provided.

Comment thread opal/corelib/class.rb
var object = #{allocate};
var object;

if (self.$$bridge && self.$allocate == Opal.Class.prototype.$allocate) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What if... we subclass a bridged class?

class Array < `Array`; end
class MyArray < Array; end
class YourArray < MyArray; end

(The lack of awareness of that edge case caused in the past a hard-to-debug issue where we missed backtraces in Firefox - due to not launching a constructor - if Exception was subclassed more than once - see #2540 ).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For comparison, we have something that's called pristine methods. This is an option for us to define a method to be unmodified from the default implementation. Perhaps this is a better idea.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Alright, i will pay attention to that case, i still need to understand better whats going on

@hmdne
Copy link
Copy Markdown
Member

hmdne commented Sep 18, 2023

Also, take a look at the prior work on this matter:

#2344

@janbiedermann
Copy link
Copy Markdown
Member Author

So the last patch makes bridge for subclasses work and no longer cuts of the prototype chain

class A { a() {return 'a'}}
class B extends A { b(){return 'b'}}
class C extends B { c(){return 'c'}}
Opal.klass(Opal.Kernel, C, 'C')

d = Opal.Kernel.C.$new()
d.c() -> 'c' - worked before
d.b() -> 'b'  - before b() was gone, now works
d.a() -> 'a' - before a() was gone, now works

There are still issues though

@janbiedermann
Copy link
Copy Markdown
Member Author

janbiedermann commented Sep 20, 2023

At this stage the following works:

require 'engine'
# Engine now allow access to globalThis, so in the browser for example
Engine.history.length # -> 2

manually in the browser console in js:

Engine.$send('history').$send('length') // -> 2

#send is required, because method_missing is used

@janbiedermann
Copy link
Copy Markdown
Member Author

further works:

require 'engine'
Engine.history.class.name # -> Engine::History

a = Engine::ArrayBuffer.new()
a.class.name # -> Engine::ArrayBuffer

the last one in js:

Opal.Engine.$const_get('ArrayBuffer').$new().$class().$name() // -> 'Engine::ArrayBuffer'

Comment thread stdlib/engine.rb Outdated
message = message.chop
property_name = ::Engine.property_for_message(message, self)
arg = args[0]
arg = arg.to_n if `arg && typeof arg.$to_n === 'function'`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what if $to_n is a stub? It's still a function then.

@hmdne
Copy link
Copy Markdown
Member

hmdne commented Sep 20, 2023

I kind of fail to understand what Opal::Engine does. Is it yet another wrapping solution? Wouldn't it be a better idea to split that to another PR?

@janbiedermann janbiedermann marked this pull request as draft September 20, 2023 12:19
@janbiedermann
Copy link
Copy Markdown
Member Author

Possibly, its handy for testing things, as its the goal to have automatic, easy and compact access to JS APIs and values. Once everything is working i may make that another PR or not, we will see

@janbiedermann
Copy link
Copy Markdown
Member Author

For correctness when using Engine for testing the bridges proeprties must be accessed by [], eg:

Engine[:history]
Engine[:history][:length]
Engine[:history].back

May work more or less, anymway
Continuing from here ...

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