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

replace quickjs with other js engine #415

Merged
merged 7 commits into from
Oct 12, 2022
Merged

Conversation

L0rnwizd7
Copy link
Contributor

quickjs could not be used in windows, also could not be load in linux system. replace it with jdk8's js engine to excute js code

@L0rnwizd7 L0rnwizd7 closed this Oct 9, 2022
@L0rnwizd7 L0rnwizd7 reopened this Oct 9, 2022
@Syer10
Copy link
Collaborator

Syer10 commented Oct 9, 2022

This is a good idea, have you tested it with ReadComicOnline and Mangahere? They are 2 sources which evaluate JS

Comment on lines 18 to 30
public Object evaluate(String script, String fileName) {
try {
return engine.eval(script);
} catch (Exception exception) {
exception.printStackTrace();
return "";
}
}

public Object evaluate(String script) {
try {
return engine.eval(script);
} catch (Exception exception) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to swallow the error? I think that the sources would just error anyway if it returns a empty string since its not the expected output.

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 test it with manhuagui extension and it work well. But i find Readcomiconline extension will use quickjs's compile function and i have no idea about how this function works.
image

And you are right, I should not swallow the error, may need to throw another runtime exception. I just want the function signature to be same with quickjs.

server/build.gradle.kts Outdated Show resolved Hide resolved
@AriaMoradi
Copy link
Member

We are using Mozilla Rhino in Duktape implementation already, maybe it could also replace quickjs

@Syer10
Copy link
Collaborator

Syer10 commented Oct 9, 2022

The Duktape implementation can be removed, but we could use Rhino in the QuickJS implementation

@L0rnwizd7
Copy link
Contributor Author

I tested with manhuagui, tencentcomics and readcomiconline and all work well. But mangahere will throw NullPointerException which it's quite weird....

@L0rnwizd7 L0rnwizd7 changed the title replace quickjs with jdk 8 default js engine [WIP] replace quickjs with other js engine Oct 10, 2022
@L0rnwizd7
Copy link
Contributor Author

I tested some mangas in mangahere, only the below manga will throw nullpointer exception. maybe it's the extension's problem.
image

@AriaMoradi
Copy link
Member

AriaMoradi commented Oct 10, 2022

We could keep QuickJS and substitute it with Rhino whne we are missing libc++?

@Syer10
Copy link
Collaborator

Syer10 commented Oct 10, 2022

We could keep QuickJS and substitute it with Rhino whne we are missing libc++?

That would be difficult to do, extensions instantiate QuickJS directly. We can't really do much about that unless we add a redirect like we did with SimpleDateFormat

Also I think we should move this file to the AndroidCompat module, since its meant to replace a dependency like the other things there.

@Syer10
Copy link
Collaborator

Syer10 commented Oct 11, 2022

I did some testing with the QuickJs sources, here are the results

Comicabc(zh) java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.Integer
Mangahere(en) Works
Readcomiconline(en) Works
QiXiManhua(zh) Works
TencentComics(zh) Works
Manhuagui(zh) Works

You probably need to add Long to Int in the translateType function

@AriaMoradi AriaMoradi changed the title [WIP] replace quickjs with other js engine replace quickjs with other js engine Oct 11, 2022
@AriaMoradi AriaMoradi merged commit b1bf901 into Suwayomi:master Oct 12, 2022
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.

3 participants