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

Add RequireRemote#load to load external Ruby scripts from server #292

Merged
merged 4 commits into from
Dec 26, 2023

Conversation

ledsun
Copy link
Contributor

@ledsun ledsun commented Oct 9, 2023

Motivation

To run a ruby.wasm application consisting of multiple Ruby scripts, the developer needs to enumerate multiple Ruby scripts.
Like below:

<script type="text/ruby" src="lib_a"></script>
<script type="text/ruby" src="lib_b"></script>
<script type="text/ruby" src="main"></script>

I want to automatically resolve dependencies of Ruby scripts according to the definition of require_relative.

In main.rb:

require_relative 'lib_b'
...

In lib_a.rb

require_relative 'lib_b'
...

Feature

This pull request provides RequireRemote#load method.
RequireRemote#load loads Ruby scripts from server.
This is used to emulate require_relative.
The simplest expamle is below:

require 'js/require_remote'

module Kernel
  def require_relative(path) = JS::RequireRemote.instance.load(path)
end

The argument of the RequireRemote#load method is treated as a relative URL, and the dependent ruby script is fetched from that URL.
For example, RequireRemote#load 'lib_a' will fetch the script from the URL ./lib_a.rb.

ext/js/lib/js/browser.rb Outdated Show resolved Hide resolved
Copy link
Member

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

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

Thank you for working on the feature and sorry for not responding for a while 🙇
Several major comments:

  1. Can we use another name for this feature to avoid patching require_relative? The method is already patched by several places and I don't want to introduce new complexities here. How about require_remote?
  2. I'd like to make this feature opt-in since fetch is a part of Web API and not generally available on non-Web JS runtimes. Could you remove require_relative "js/loader.rb" statement from js.rb and let users explicitly require the file by require "js/loader"?
  3. What happens if "./foo.rb" is redirected to "./bar.rb" at HTTP level and a user tries to load both? This is similar to file system symlink with require_relative and it loads only one of them.
  4. I think, loader is a little bit too general word. To express it loads remote contents, how about js/remote_loader.rb, or js/require_remote.rb?

@ledsun
Copy link
Contributor Author

ledsun commented Oct 31, 2023

Thank you for your comment. I will answer my current thoughts.

Can we use another name for this feature to avoid patching require_relative? The method is already patched by several places and I don't want to introduce new complexities here. How about require_remote?

I want to run Ruby scripts that work with CRuby in ruby.wasm without modification. Therefore, I would be happy if require_relative works as is. However, I have some concerns about patching the underlying API.
I think it is possible for ruby.wasm to provide this function under a name other than require_relative and replace require_relative with a patch outside ruby.wasm.

However, considering that 'require' will be replaced in the future, a name other than require_remote would be better. For example, require_relative_remote. However, I think it is a bit uncool.

I'd like to make this feature opt-in since fetch is a part of Web API and not generally available on non-Web JS runtimes. Could you remove require_relative "js/loader.rb" statement from js.rb and let users explicitly require the file by require "js/loader"?

I think it is a reasonable idea. I will correct it.

What happens if "./foo.rb" is redirected to "./bar.rb" at HTTP level and a user tries to load both? This is similar to file system symlink with require_relative and it loads only one of them.

The current implementation does not handle redirect responses. If a redirect response is returned, it simply fails.
I believe it is possible to implement identification by the pre-redirect URL.

Before that, there is a missing implementation that returns false without loading when require_relative is run multiple times on the same URL. I intend to implement this.

I think, loader is a little bit too general word. To express it loads remote contents, how about js/remote_loader.rb, or js/require_remote.rb?

Okay, I will rename the Loader.

If you have any ideas, please let me know again.

@kateinoigakukun
Copy link
Member

However, considering that 'require' will be replaced in the future, a name other than require_remote would be better. For example, require_relative_remote. However, I think it is a bit uncool.

How about require_relative "foo.rb", remote: true?

@ledsun ledsun force-pushed the require_relative branch 2 times, most recently from f576fe8 to 8eebd6c Compare November 5, 2023 02:24
@ledsun ledsun force-pushed the require_relative branch 2 times, most recently from 95e7afe to 9a66904 Compare November 19, 2023 03:36
@ledsun ledsun changed the title Load external Ruby scripts from the browser with require_relative. Add RequireRemote#load to load external Ruby scripts from server Nov 19, 2023
@kateinoigakukun
Copy link
Member

If you face build stuck on your local, please rebase upon the latest main 🙏

@ledsun
Copy link
Contributor Author

ledsun commented Nov 22, 2023

Thanks for letting me know. I was stuck in the following part of the build and it has been resolved.

==> make -j1 install_sw DESTDIR=/home/ledsun/ruby.wasm/build/wasm32-unknown-wasi/openssl-3.0.5/opt

@ledsun ledsun force-pushed the require_relative branch 2 times, most recently from 776be0c to 3de3b98 Compare November 27, 2023 00:03
@ledsun ledsun force-pushed the require_relative branch 3 times, most recently from 78f9a70 to ac43be2 Compare December 4, 2023 13:06
@ledsun ledsun force-pushed the require_relative branch 3 times, most recently from 8ea928e to af4a56d Compare December 23, 2023 10:48
@ledsun ledsun marked this pull request as ready for review December 23, 2023 11:01
Users use JS::RequireRemote#load. The user uses this method to replace the require_relative method.
Fix some test codes.
- Extract a custom router for integration tests
- Split integration test files
- Files the body returned by the proxy server
Copy link
Member

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

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

Would you mind adding unit test also to cover more cases? I wonder what should happen for the following case:

.
├── lib
│   ├── a.rb -- require_relative "b.rb"
│   └── b.rb
└── main.rb  -- require_relative "lib/a.rb"

Is the current implementation able to load b.rb well?

@ledsun
Copy link
Contributor Author

ledsun commented Dec 24, 2023

Added test case to run require_relative recursively.
Please let me know if there are other test cases that you think we should add.


# Return a URL object of JavaScript.
def resolve(relative_filepath)
JS.global[:URL].new relative_filepath, @url_stack.last
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to carefully handle relative path -> absolute path resolution here. IIUC JavaScript's new URL(path, base) does not behave as we expect for Kernel#require_relative (and also File.expand_path).

Here is an example:

path, base File.expand_path(path, base) new URL(path, "https://example.com" + base)
path="b.rb", base="/lib" /lib/b.rb https://example.com/b.rb
path="./b.rb", base="/lib" /lib/b.rb https://example.com/lib/b.rb

I think the test suite of File.expand_path would be helpful for you to see what should be cared https://github.com/ruby/ruby/blob/master/spec/ruby/core/file/expand_path_spec.rb

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 believe this behavior of the JavaScript's URL constructor is as expected.

In most cases, I assume that the first value for base is the URL of an HTML file, such as http://exapmle.com/index.html, or a URL such as http://exapmle.com/index without the extension.
Also, once the ruby script is loaded, the base value will be the URL of the ruby script, such as http://exapmle.com/a.rb.

So we want the string after the last / in the base value to be ignored.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok. I misunderstood the second argument. If the second argument is always a URL to a file, then it makes sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, unit tests covering this area would be preferable.

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 wrote unit testsfor JS::RequireRemote::URLResolver.
Please let me know if there are any tests that you think we should add.

@kateinoigakukun kateinoigakukun merged commit 6acf403 into ruby:main Dec 26, 2023
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