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

feat: URL & URLSearchParams #1801

Merged
merged 9 commits into from
Mar 28, 2024
Merged

feat: URL & URLSearchParams #1801

merged 9 commits into from
Mar 28, 2024

Conversation

triniwiz
Copy link
Member

No description provided.

@cla-bot cla-bot bot added the cla: yes label Jan 14, 2024
@anonrig
Copy link

anonrig commented Jan 17, 2024

🚀

@shirakaba
Copy link

shirakaba commented Jan 19, 2024

(Link to the equivalent iOS PR)

This looks way simpler and smaller than the WinterCG PR; I love it. Will be able to review properly later.

@triniwiz Could you explain what precisely this PR does? It appears to add code into test-app, so – being unfamiliar with this repo – I'm unclear whether it would add support for URL into the Android runtime itself, or just makes a proof-of-concept.

Could also you include the licence for Ada, and make clear in the repo the ownership of the code in test-app/README.md? If you can also tell me in the conversation, it will save me from reviewing @anonrig's source which has already been through thorough review.

What would this PR mean for the WinterCG repo, by the way? It seemed like the WinterCG plugins approach would allow people to opt-in to the functionality, and I'm just trying to determine whether this approach would mean we'd have it baked in by default instead.

@anonrig Thanks for engaging with the team! Hope to get a chance to chat with you at some point about cross-foundation collaboration, JS engine/runtime interop and spec conformance 🙂 I'm the new TSC Chair and all of these things excite me, so hope to advance these efforts.

@anonrig
Copy link

anonrig commented Jan 19, 2024

Can you make sure native script relies on the official Ada rust crate (ref: ada-url available at GitHub.com/ada-url/rust) instead of a fork?

Thanks @shirakaba. Nice to meet with you.

@triniwiz
Copy link
Member Author

This adds a wrapper around the url lib Ada, introducing the URL & URLSearchParams class directly to the runtime.

Doing it this removes the extra step needed to install/setup as it will be available on global.

As for the licence sure I'll need to copy it, I only followed the instructions on the repo 😅 .

The PR in the WCG repo we can remove it 🤷🏽, but yes we would lost the opt-in functionality as it will now be baked in by default .

Copy link

@shirakaba shirakaba left a comment

Choose a reason for hiding this comment

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

Thanks as always; I think we're getting real close! Some initial comments before I get in too deep:

  1. Could this be done as a plugin?
  • This would allow it to be opt-in. Maybe URL itself isn't so controversial to bake into the runtime, but by the time we come to implementing Intl, we need to have established a flexible pattern for adopting APIs from other JS engines/runtimes.
  1. Could this be done without coupling to V8?
  • This would allow us to re-use the work instantly in the future NativeScripts that may not be based on V8. We can probably still make it go adequately vroom-vroom without V8 fast API.

Choose a reason for hiding this comment

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

Nice to see tests 😍

Choose a reason for hiding this comment

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

Could we run a formatter like Prettier over this? Even if it's not integrated into the repo, would just improve readability as a one-time pass.

Choose a reason for hiding this comment

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

Another good one to run Prettier formatting on.



it("Test URLSearchParamsImpl keys", function(){
const params = new URLSearchParamsImpl("foo=1&bar=2");

Choose a reason for hiding this comment

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

As this line appears identical in each test, there may be some merit in hoisting the params variable up to the describe block, or alternatively initialising it in a before or beforeEach hook, but it's not a blocker.

Comment on lines 594 to 661
auto blob_methods =
"const BLOB_STORE = new Map();\n"
"URL.createObjectURL = function (object, options = null) {\n"
"try {\n"
"if (object instanceof Blob || object instanceof File) {\n"
"const id = java.util.UUID.randomUUID().toString();\n"
"const ret = `blob:nativescript/${id}`;\n"
"BLOB_STORE.set(ret, {\n"
"blob: object,\n"
"type: object?.type,\n"
"ext: options?.ext,\n"
"});\n"
"return ret;\n"
"}\n"
"} catch (error) {\n"
"return null;\n"
"}\n"
"return null;\n"
"};\n"
"\n"
"URL.revokeObjectURL = function (url) {\n"
"BLOB_STORE.delete(url);\n"
"};\n"
"\n"
"const InternalAccessor = class {};\n"
"\n"
"InternalAccessor.getData = function (url) {\n"
"return BLOB_STORE.get(url);\n"
"};\n"
"\n"
"URL.InternalAccessor = InternalAccessor;\n"
"Object.defineProperty(URL.prototype, 'searchParams', {\n"
"get() {\n"
"if (this._searchParams == null) {\n"
"this._searchParams = new URLSearchParams(this.search);\n"
"Object.defineProperty(this._searchParams, '_url', {\n"
"enumerable: false,\n"
"writable: false,\n"
"value: this,\n"
"});\n"
"\n"
"this._searchParams._append = this._searchParams.append;\n"
"this._searchParams.append = function (name, value) {\n"
"this._append(name, value);\n"
"this._url.search = this.toString();\n"
"};\n"
"\n"
"this._searchParams._delete = this._searchParams.delete;\n"
"this._searchParams.delete = function (name) {\n"
"this._delete(name);\n"
"this._url.search = this.toString();\n"
"};\n"
"\n"
"this._searchParams._set = this._searchParams.set;\n"
"this._searchParams.set = function (name, value) {\n"
"this._set(name, value);\n"
"this._url.search = this.toString();\n"
"};\n"
"\n"
"this._searchParams._sort = this._searchParams.sort;\n"
"this._searchParams.sort = function () {\n"
"this._sort();\n"
"this._url.search = this.toString();\n"
"};\n"
"}\n"
"return this._searchParams;\n"
"},\n"
"});";

Choose a reason for hiding this comment

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

We could make this more legible by changing it to a raw string literal, which has been available since C++11 (would that standard be supported without any changes to the build?):

Suggested change
auto blob_methods =
"const BLOB_STORE = new Map();\n"
"URL.createObjectURL = function (object, options = null) {\n"
"try {\n"
"if (object instanceof Blob || object instanceof File) {\n"
"const id = java.util.UUID.randomUUID().toString();\n"
"const ret = `blob:nativescript/${id}`;\n"
"BLOB_STORE.set(ret, {\n"
"blob: object,\n"
"type: object?.type,\n"
"ext: options?.ext,\n"
"});\n"
"return ret;\n"
"}\n"
"} catch (error) {\n"
"return null;\n"
"}\n"
"return null;\n"
"};\n"
"\n"
"URL.revokeObjectURL = function (url) {\n"
"BLOB_STORE.delete(url);\n"
"};\n"
"\n"
"const InternalAccessor = class {};\n"
"\n"
"InternalAccessor.getData = function (url) {\n"
"return BLOB_STORE.get(url);\n"
"};\n"
"\n"
"URL.InternalAccessor = InternalAccessor;\n"
"Object.defineProperty(URL.prototype, 'searchParams', {\n"
"get() {\n"
"if (this._searchParams == null) {\n"
"this._searchParams = new URLSearchParams(this.search);\n"
"Object.defineProperty(this._searchParams, '_url', {\n"
"enumerable: false,\n"
"writable: false,\n"
"value: this,\n"
"});\n"
"\n"
"this._searchParams._append = this._searchParams.append;\n"
"this._searchParams.append = function (name, value) {\n"
"this._append(name, value);\n"
"this._url.search = this.toString();\n"
"};\n"
"\n"
"this._searchParams._delete = this._searchParams.delete;\n"
"this._searchParams.delete = function (name) {\n"
"this._delete(name);\n"
"this._url.search = this.toString();\n"
"};\n"
"\n"
"this._searchParams._set = this._searchParams.set;\n"
"this._searchParams.set = function (name, value) {\n"
"this._set(name, value);\n"
"this._url.search = this.toString();\n"
"};\n"
"\n"
"this._searchParams._sort = this._searchParams.sort;\n"
"this._searchParams.sort = function () {\n"
"this._sort();\n"
"this._url.search = this.toString();\n"
"};\n"
"}\n"
"return this._searchParams;\n"
"},\n"
"});";
auto blob_methods = R"js(
const BLOB_STORE = new Map();
URL.createObjectURL = function (object, options = null) {
try {
if (object instanceof Blob || object instanceof File) {
const id = java.util.UUID.randomUUID().toString();
const ret = `blob:nativescript/${id}`;
BLOB_STORE.set(ret, {
blob: object,
type: object?.type,
ext: options?.ext,
});
return ret;
}
} catch (error) {
return null;
}
return null;
};
URL.revokeObjectURL = function (url) {
BLOB_STORE.delete(url);
};
const InternalAccessor = class {};
InternalAccessor.getData = function (url) {
return BLOB_STORE.get(url);
};
URL.InternalAccessor = InternalAccessor;
Object.defineProperty(URL.prototype, "searchParams", {
get() {
if (this._searchParams == null) {
this._searchParams = new URLSearchParams(this.search);
Object.defineProperty(this._searchParams, "_url", {
enumerable: false,
writable: false,
value: this,
});
this._searchParams._append = this._searchParams.append;
this._searchParams.append = function (name, value) {
this._append(name, value);
this._url.search = this.toString();
};
this._searchParams._delete = this._searchParams.delete;
this._searchParams.delete = function (name) {
this._delete(name);
this._url.search = this.toString();
};
this._searchParams._set = this._searchParams.set;
this._searchParams.set = function (name, value) {
this._set(name, value);
this._url.search = this.toString();
};
this._searchParams._sort = this._searchParams.sort;
this._searchParams.sort = function () {
this._sort();
this._url.search = this.toString();
};
}
return this._searchParams;
},
});
)js";

@triniwiz triniwiz marked this pull request as ready for review January 20, 2024 03:32
@farfromrefug
Copy link
Contributor

farfromrefug commented Jan 21, 2024

I personally think we should make that an agnostic plugin.
there is no real gain in accessing v8 direclty here, like you dont do 1000 URL use per second or something.
we could compare perfs with using JNI but I dont think it would make much of a difference (the only addition is just the JNI layer).
We could also set up a plugin repo uing swig to create bindings to JAVA / Objc (or something else if there is better). swig is nice, I need to check if latest versions add less overhead than before.
there would be one drawback though. it would not support out of the box future platforms like desktop. but does this ?

@NathanWalker NathanWalker merged commit 4f3a0d7 into main Mar 28, 2024
1 of 3 checks passed
@NathanWalker NathanWalker deleted the feat/ada-url-y-search-params branch March 28, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants