ondras / wwwsqldesigner

WWW SQL Designer, your online SQL diagramming tool
https://sql.toad.cz/?keyword=online_library
BSD 3-Clause "New" or "Revised" License
2.88k stars 729 forks source link

love sql designer #329

Open dataf3l opened 5 months ago

dataf3l commented 5 months ago

used it for many years! so this "var ai=" thing may have a problem with upcoming changes on the browser, as it hides window.ai maybe it should be renamed.

https://github.com/ondras/wwwsqldesigner/blob/a8be2f2197f84f5427d6b5671470140af6280f02/js/row.js#L392

have an awesome day!

ondras commented 4 months ago

Hello @dataf3l,

can you please elaborate a bit on the upcoming changes on the browser? I am not aware of any API that surfaces via the window.ai global variable. Moreover, the variable defined at https://github.com/ondras/wwwsqldesigner/blob/a8be2f2197f84f5427d6b5671470140af6280f02/js/row.js#L392 is local and not global.

dataf3l commented 4 months ago

Hi Ondras! I love your software since 2009 or so, I am surprised by the dedication to continue to improve on it! I used it many times, it was always a pleasant experience, compared to the tools available at the time!

  1. The name of the thing is built in ai https://developer.chrome.com/docs/ai/built-in (sadly no code snippet is there, but like, it's coming, some discussion about that can be read here https://news.ycombinator.com/item?id=40834600
  2. The local variable hides the ai object for code further down the line in the same scope which may (as in, like, maybe) require it
  3. Such code may exist in the future, hypothetically, in many applications that receive text from users
  4. It's similar to saying var navigator = xyzzy and then wondering why further down the line the navigator.userAgent object suddenly doesn't work and returns undefined, it may lead to silent bugs, so I say let's leave the variable name to be owned by the browser.

have an awesome day

ondras commented 4 months ago

The local variable hides the ai object for code further down the line in the same scope which may (as in, like, maybe) require it

Not necessarily. You can always access global variables via the window prefix.

If I was to follow your suggestion to the point, I would not be able to use any local variables that share names with existing global variables, such as name, length, location, top and many more.

Such code may exist in the future, hypothetically, in many applications that receive text from users

Right. So in theory we are forbidden to use any variable name, because in the future, a standardized global API with the same name might exist? :smile:

The point here is that our local variable will still work, even with a window.ai global. And if someone needs to use window.ai in the very same function that already declares an ai local variable, they can always

  1. use the window.ai access,
  2. rename the local variable once the collision inside the function is inevitable

But this is not the case, as we do not use the new Chrome API inside SQL.Row.prototype.toXML.

dataf3l commented 4 months ago

Hi Ondras!

I tried this code to test out the idea, and sure enough, your ideas are 100% correct.

function f(){  var ai = 1; console.log(window.ai); }

If we consider the main post of this github issue to raise awareness about this new possibly conflicting change, then I assume my job here is done, thank you for your attention.

you can probably close this issue

Having said that, if time is available, after reading your thoughtful response, this came to my mind.

Oh and by the way, it looks like you are a person who enjoys thinking, I love that!

"Not necessarily. You can always access global variables via the window prefix."

In the past, web developers have always chosen the shortcut, people rarely use window.alert() when alert() is present.

suppose line 1 defines alert = 1, then maybe line 200 will fail, assuming a 200 line method.

Consider the word document, people rarely use window.document.getElementById, instead they use document.getElementById (or, at least they used to, before the React/Angular days anyway)

Consider the new developers, in a distant future, modifying and improving on any particular piece of code. Will they most likely choose (the, longer) window.ai.createTextSession() ? or will they instead most likely choose the least effort-ish version of the same thing, ai.createTextSession() ? see how I didn't type all of the very many seven (count them, seven in total) characters required to say "window." ? I suspect upcoming developers in the year 2032 will do exactly that, either by laziness or by efficiency, both normal traits in developers.

unsuspectingly, the developer will stumble upon the fact of "error, undefined" which will still, in 2032 be a problem in browsers, (they don't evolve that fast), the newbie will be stumped, and unable to continue (unless he uses AI to solve it, but that is a whole other question here).

so line 1 says ai=xxx and line 200 says ai.createTextSession(), indicating

"ai.createTextSession" is not a function

, as we know, not all browsers will have it, so most likely the developer will provide a soft failure.

This is because the developer more likely to be lazy and avoid the window. prefix than the developer is likely to have every single variable name in every single local scope.

So, I come to the conclusion, that if a piece of code is big and complex, and memory is limited, people may waste time, and as a result, I see all uses of "ai=xxx" and also "window.ai=xxx" to be potential problems.

Sure, your method isn't 200 lines today, but in the future, it may grow, in particular if it's AI itself writing these long methods, as is it's current incarnation, commonly long sections of code, not short ones.

So, perhaps my worries are non-existant, but I still think the new Chrome will break many websites.

"Such code may exist in the future, hypothetically, in many applications that receive text from users"

What I meant by "hypothetically" was that people will want/need window.ai integration in their text inputs, not just here or there, but like, everywhere, as people might, not that there will be hypothetical variables, but rather, hypothetical demand for AI, which may or may not be there.

Here is a chart for AI integration interest in google, I don't think it proves anything, but it's another datapoint to consider.

https://trends.google.com/trends/explore?date=today%205-y&q=ai%20integration&hl=en

So sure, right now, you understand perfectly the issue, but in the future, will people have your same understanding of the source code details?

"If I was to follow your suggestion to the point, I would not be able to use any local variables that share names with existing global variables, such as name, length, location, top, ..."

I actually think you have just made an excellent point here!!

and I also think you are correct, in that people may have introduced bugs in the source code like this (consider this hypothetical timeline):

it is reasonable to assume people changing line 1 and 2 did not read line 1000. it is also reasonable to assume that people don't expect us to "overwrite system words".

therefore, even if "name" and "length" aren't very commonly used variables, I do think overwriting location may have (and given how many developers exist, this may be substantial), introduced already subtle rare bugs, which only take place some times, most likely due to the lack of TypeScript usage.

I also think overwriting ai will have the exact same effect.

So, yes, this is in fact the exact same case, and I suspect instances of this bug exist right now as we speak (hopefully, in other people's code).

Thank you for your thoughtful response and for your attention, please keep making wwwsqldesigner awesome!

ondras commented 4 months ago

Right, software engineering is not an easy task. Fortunately, we have some useful tools and principles on our hand:

1) If you decide to modify a function, it is generally recommended to familiarize yourself with the function. If you decide to modify stuff, you should understand how that stuff works. Adjusting a function without the full knowledge about that particular function will be always risky, name-collisions aside.

2) There are guidelines regarding function lengths, see https://medium.com/swlh/how-long-should-functions-be-how-do-we-measure-it-cccbdcd8374c, https://medium.com/@martin.omander/i-like-uncle-bobs-guidance-on-function-length-eda952a092e9 or https://dzone.com/articles/rule-30-%E2%80%93-when-method-class-or. These limits are there to encourage the developer's capability of comprehending the whole function at once -- this includes the knowledge about local variables, their types and values.

3) Every single addition to a code base shall be tested, manually and/or automatically. This would certainly prevent the ai problem this issue is about.

4) Ideally, we should be using proper type-based tooling (think TypeScript) in order to make decisions about variable types. WWWSQLDesigner's codebase is old and I have no time to convert it to TS, unfortunately. But generally speaking, that would be the optimal way to go: once we switch to TS, your ai issue becomes non-existant, because the TS compiler would then warn you about type incompatibility.

5) Who holds the responsibility? Always the one who is making changes. So if the current codebase (that includes an ai local variable) works fine, it is up to the future dev who is adding support for window.ai to make sure his ai.stuff() works as expected.