Skip to content

Conversation

ledsun
Copy link
Contributor

@ledsun ledsun commented Sep 7, 2024

Problem

require 'js'
JS.global[:document].undefinded_method?

This code will cause the following error.

RbError: /bundle/gems/js-2.6.2.dev/lib/js.rb:176:in 'JS::Object#call': TypeError: Cannot read properties of undefined (reading 'undefinded_method')

This is because the JavaScript function existence check is missing.

Solution

Checks for the existence of JavaScript functions.

@ledsun ledsun marked this pull request as ready for review September 7, 2024 12:12
@kateinoigakukun
Copy link
Member

kateinoigakukun commented Sep 8, 2024

On the second thought, I hesitate to introduce additional semantics differences with/without '?' suffix. I think the root problem here might possibly be the unclear error message thrown when calling non-existing methods.

If the error message is something like TypeError: document.undefinded_method is not a function, is your initial problem satisfied?

@ledsun
Copy link
Contributor Author

ledsun commented Sep 8, 2024

I see.
It is strange that it returns false only when call properties with '?'.
It would be better to have the same error when calling a property with/without '?'.

@kateinoigakukun
Copy link
Member

kateinoigakukun commented Sep 8, 2024

I'll have a time to improve error diagnostic messages anyway:)

@ledsun
Copy link
Contributor Author

ledsun commented Sep 8, 2024

Fixed as follows.
When we call a JavaScript property as a method, we get a NoMethodError.

Perhaps it would be better to;

  • raise TypeError when the property is called
  • raise NoMethodError when the property is undefined

One thing I am wondering about is the behavior when an existing JavaScript property has an undefiend.

@ledsun ledsun force-pushed the check_js_function_with_question_mark branch from 25aeb94 to 84561c3 Compare September 30, 2024 09:29
@ledsun ledsun marked this pull request as draft September 30, 2024 09:30
@ledsun ledsun closed this Sep 30, 2024
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