Updating onConnect() to a new API #293
Comments
|
Sorry if I don't reply to all issues and PR right away in the coming weeks. Don't worry, I'll reply, probably the weekend. :) |
|
@vincentfretin no worries, I'm also pretty busy. It's fine having it async and just moving along when it moves. I'll just keep doing my thing, get to it when you can. |
eval is not a good idea, and there are security implications of using eval on an unknown string. This is worse than the global onConnect. :) I think we already have enough deprecation warnings and breakage in THREE. If we can avoid breakage in aframe libraries that are not related to THREE, I think we developers using those libs would be happier. :D
From reading the code, I think you can just use today It should work properly even with is executed only if you try to connect a second time I think?
Well, in the end, having (You absolutely need to have something other that onConnect otherwise it will be executed twice, if we don't change the networked-scene API) instead of I don't see much value to change the examples.
For me it's not a good reason to introduce a breaking change. The current API is no so bad. :) |
|
In the documentation we currently have |
We are not running eval on some other user's text input... There's no reason to be paranoid about
I disagree. Non-namspaced globals are a terrible idea, and a global named That said, I get why it's mildly unpleasant, but it's a result of the A-Frame decision to allow declaration of everything within HTML attributes, which is inherently awkward. If you take a second to think about it, this is actually more or less identical to the existing So, to my point: this is consistent with both existing HTML and A-Frame precedent.
There are many more types than that, and you can declare custom component schema inputs 1. I've used this custom component schema to accept JSON in another component I wrote, and the same principle could be used to eval function inputs instead.
The fact that THREE abuses the logs is not an argument for us to not use them reasonably. Until THREE fixes their log issue, which they've finally acknowledged they should, I recommend all users silence THREE logs using a snippet similar to this: // TO TURN OFF THREEJS LOGS
vrgc.console = {
// log: console.log,
info: console.info,
warn: console.warn,
error: console.error,
counters: {
}
};
Object.keys(vrgc.console)
.forEach(
key => {
console.warn(`hiding console.${key} calls from THREE`)
vrgc.console.counters[key] = 0;
console[key] = function() {
if ( (typeof arguments[ 0 ] === 'string') && (arguments[ 0 ].substr( 0, 5 ) === 'THREE') ) {
// ignore noisy THREE, but check here if something breaks.
vrgc.console.counters[key]++
return
}
else {
vrgc.console[key].apply( console, arguments );
}
}
}
);Meanwhile, we're pre 1.0, and this is a very easy 'breaking change' to detect, notify, and for devs to fix. There's no reason for this to be painful at all. All this said, if you don't see why this needs changing, I'm not sure what to say. To me it's clear, and the first time I saw this, I was pretty shocked and it made it looked very half-baked and like a very undeveloped library. Again:
|
|
I agree I may have been in the "eval is evil" position. After reading your reply and some comments with the link you gave, I think it's ok. You pointed out we could do a custom field type for aframe schema, I didn't do the search, I didn't know that. I absolutely agree with you on the global namespace pollution and potential collision. I'm not convinced on writing all your connect function code as a string. It depends on what you want to do in this callback, but it may be several 10 or 50 lines of code. If instead of all those lines of code, you just use your function name, you come back of using a function in the global namespace. Writing code in a string usually means you don't have your IDE recognize that as code and so you don't have the syntax errors, linting etc. I would be much in favor of something like this: or For the "connected" event emitted on document.body, I didn't consider the potential collision issue with other libs. I agree we could rename it to "naf-connected". I know we are on pre 1.0.0 version and with semantic versioning we can introduce breaking changes in 0.x.0. |
|
If we're going to break compatibility with aframe < 1.3.0 because of #280 then let's do all those breaking changes, properly document those in the release notes and add a warning in the README we don't support aframe < 1.3.0 and you need to use naf 0.8.x for older aframe versions. |
So, to be clear, I'm not suggesting writing the function as a string should be the primary way of doing this, just that we enable that for consistency. What I mean is that you can declare an <div id="mydiv" onclick="doSomething()"></div>vs. // assuming doSomething = () => { /* stuff */ }
const div = document.querySelector('#mydiv')
// sadly, this HTML interface also requires adding a string, for consistency:
div.setAttribute('onclick','doSomething()')
// this is nicer, though:
div.onclick = doSomethingThe normal way would be to do the function declaration in JS, just as I believe that while one can declare <a-scene networked-scene="onconnect: alert('connected'); room: default; audio:true"></a-scene>
// ^this matches the HTML onclick pattern, but we could also do this:
<a-scene networked-scene="onconnect: () => {alert('connected')}; room: default; audio:true"></a-scene>ideally, you would do this: const scene = document.querySelector('a-scene')
scene.setAttribute('networked-scene', {
onconnect: function() { alert('connected') },
room: 'default',
audio: true,
}
// in theory, I would also propose the custom attribute should allow this:
scene.setAttribute('networked-scene','onconnect',() => { alert('connected') })
// but that would frankly not make much sense in this specific context, since you would specify onconnect's behavior at the time you add networked-scene to a-scene, and it wouldn't generally safely be expected fire if you added it later for some reason.A-Frame has this pattern of being able to specify stuff in HTML with a bunch of stuff that gets parsed as strings. I'm just saying we match that A-Frame pattern. If you want to do a 50-line onConnect function, no problem, just specify it in a component and assign it whenever you're ready to add the |
|
Ok I understand now the consistency with the Note that because If we want consistency, first please note that does nothing. does something. The attribute value is evaluated when the user click the button. Similarly does nothing. shows the alert dialog. You can play with the onclick attribute here: https://www.w3schools.com/jsref/event_onclick.asp The syntax is quite limited because you can't use semicolon in or simply like your first example
As a developer I would maybe want I tested that now, I thought it would give me an error, but it just fail to parse silently without errors. gives gives Adding semicolon after the alert instruction: gives If you want more that one instruction in your function, you end up using: In comparison we have this today without parentheses: The following should work but we agree that it's meaningless to write it like that, you would better write: So, with all the above said, I don't see much value changing the behavior of I'm in favor of keeping the onConnect behavior like it is today and add support for setting a function with the js syntax in the last code snippet. |
|
About the other proposal I'm afraid we could have a race condition similar to #267 because the connection logic is called at networked-scene component initialization, and the 'naf-connected' listener may not have been registered yet. |
|
I'm aware of how the native html onclick works (that it executes the string, etc.). I hope that's clear from my examples and proposal.... My point was that we don't have to have a perfect replica of onclick, though we can. Good point about the semicolons, which esssentially become overloaded here. (Too bad aframe didn't require something more clear for separating properties in strings). So, yes, to properly match the native As a result, yeah, I would understand ditching the string eval option. More opportunity to be confused by the semicolon parsing limitation than it would help if we include that in the docs.
I am still in favor of removing the call to global method option--if we do leave it until we switch to 1.0 at some point, then at least we should unpublish it as a recommended option and make it silent only for backwards compatibility. My take:
(And yes, to avoid a race condition, you should absolutely declare your listener before |
|
We can set onConnect property to empty string by default here I'm ok with that. You will need to set it explicitly if you want to use the globalonConnect function, yes.
About the race condition, if you declare your listener and after set networked-scene with javascript, you will have no issue, of course. I couldn't produce an issue though, but it doesn't mean it can't happen. Anyway, I don't think we want to promote this particular usage. or fully in javascript and not promote a third way of doing things. Do you agree? |
|
So, I'm still pretty strongly against the idea of declaring and calling globals, like this: diff --git a/examples/basic-audio.html b/examples/basic-audio.html
index 2444e2d..2b24ea4 100644
--- a/examples/basic-audio.html
+++ b/examples/basic-audio.html
@@ -16,6 +16,8 @@
</head>
<body>
<a-scene networked-scene="
+ connectOnLoad: true;
+ onConnect: doSomething;
room: basic-audio;
debug: true;
adapter: easyrtc;
@@ -141,5 +143,10 @@
});
}
</script>
+ <script>
+ function doSomething () {
+ console.log("onConnect", "hello");
+ }
+ </script>
</body>
</html>But if declaring it in JS like we've agreed upon above isn't enough, how about this as an option: we expose a promise on the NAF global, e.g., NAF.connection.active.then(doSomething)
// or
(async () => {
await NAF.connection.active
doSomething()
})()Behind the scenes, this is very simple to implement in NAF. This:
how do you feel about that? |
|
So, I read through some of the NAF source code on this, and realized I hadn't noticed the full power of in the newest draft of the pull request, I show a monkey patch on NAF to add a new promise called NAF.connection.active = new Promise((rs, rj) => { NAF.connection.onConnect(rs)})With that patch in place, we can do this: NAF.connection.active.then(() => {console.log('NAF connected at', new Date())})In theory, we could instead just use this function right now, no monkey patch needed, directly: NAF.connection.onConnect(() => {console.log('2. NAF connected at', new Date())})This already enables adding multiple functions that get called upon connection, and eliminates race conditions... with an important caveat: you must call So, my real recommendation is we add this to NAF itself, and make this promise style the new standard: NAF.connection.active.then(() => {console.log('1. NAF connected at', new Date())})
// or, in async function
await NAF.connection.active
console.log('NAF connected at', new Date())The benefit is that the existing method
And besides that... |
|
I think beginner developers may like to use a global onConnect function, it's just simple. No need to learn about closure and Promise and async/await to start tinkering with NAF. If someone doesn't like global, they can use one of the other syntaxes that doesn't need to use a global. You can indeed give a callback to NAF.connection.onConnect(() => {
console.log("connected');
});For a Promise based API, I like better the API used by the janus adapter. scene.components["networked-scene"].connect().then(() => {
console.log("connected');
})
.catch((connectError) => {
console.log(connectError);
});
}or the equivalent with async/await. No change needed in |
This is a networking library, in JS, for VR. I think it's reasonable to expect a dev to have the most basic understanding of how a promise works. There is no uniquely special need to understand scope or closure here. It's just: init: async function() {
await NAF.connection.active
doSomething()
}That's it. It's just a safer, more consistent, more reliable, more powerful option. No race conditions, no name collisions, no fancy features. You don't even have to understand how it works, you just declare your function as It's ok if you're quickly debugging to throw a global around, but the idea that a library reaches for some random global function and calls it is kind of crazy, and I just don't understand how I have to justify this. I don't know of any other reputable library that has this anti-pattern baked in and listed on the front page readme. That's the kind of thing you do when you're throwing something together, not something you do when you're thinking it out and doing it right.
I mean, that's great and all, but is just a different scenario, as well as looking much messier. For one, the user shouldn't call I think the Janus adapter's API there is great, and it should exist, but it's not a solution or an alternative to what I'm talking about here. And |
|
I'm not against a Promise API like you proposed, but this would mean a fourth way of doing things if we keep the others we talked about. |
If you go back and read, I never actually supported the use of a string pointing to a global function, though you did respond suggesting that. That would indeed be just allowing the same global function we have now, just with allowing custom names. If we must keep a global function, I'd suggest it be within the NAF namespace; e.g. I didn't comment on this suggestion of the string pointing to a global function's name because we decided to use the string because of the semicolon parsing issue--but to be clear, yes, I'm not a fan of the global function name string eval option either. Naked globals just not a good choice across the board imo, I'm fully consistent on this issue.
My suggestions are this:
|
|
Ok then, please proceed to the changes in a separate PR. Other tasks that needs to be done we also discussed here:
|
I'd propose
1a) Update the schema to not just take a function name, but an actual function (either a string that we convert into a function to eval or, if set by JS, can just be an actual function)
1b) Alternatively, update the schema to just not accept this at all
1c) Either way, include a deprecation warning if we detect user trying to use old API, a la THREE
2) Change the event to be more tightly scoped--other libraries could easily use "connected" as an event as well. Perhaps "naf-connected" should be the new event?
2a) We can continue to leave in the old "connected" event for now, along with a deprecation warning when we emit it.
2b) When we call the built-in global, if it exists, we should also emit a deprecation warning.
3) Update docs and examples to use this new pattern.
4) Update version to e.g. 0.8.4 reflect the new API we are encouraging, and update the version to 0.9.x whenever we later remove the old API to reflect the potentially breaking API update. Relying on a generically named global like this just seems like bad practice that we should actively go out of our way to remove
also, because of this
The listener should be set before NAF script is declared, yeah?
Thoughts?
The text was updated successfully, but these errors were encountered: