8 0 beta #10592

derrell · 2023-05-13T16:27:50Z

New class/property system

source/class/qx/core/propertystorage/ImmutableDataArray.js

cboulanger

Exciting! I hope I will have the opportunity to test it in the near future. Until then- as it passed the tests, I trust it's ready as it is for a breaking release

… a compatible release. also, fixes an issue where the environment check `qx.future.checkjsdoc` is now declared as `qx.Class.futureCheckJsDoc` so that the compiler can detect it and eliminate code (this fixes the require warning for jsdoctypeparser) also, blocks declaring variables called `arguments` which breaks uglify

johnspackman · 2023-06-01T18:58:49Z

I'm finding this quite hard to debug - stepping into an ordinary method called getUserAsync() does not take me to the function - instead I have to step through https://github.com/qooxdoo/qooxdoo/blob/8_0_beta/source/class/qx/Class.js#L902-L921 in order to get to my code; while stepping through the code, the values in the debugger are not what I would expect (eg the value at https://github.com/qooxdoo/qooxdoo/blob/8_0_beta/source/class/qx/Class.js#L911 is my function and not the return of the function).

While debugging, the inspector shows nothing at first:

and when the [[Target]] is expanded there is a huge array of properties in the object:

There's another 4 pages of property values, and my actual properties are hidden in the middle.

Is it the case that every method call will be wrapped with a function? What effect does the extra properties in the object have on memory usage?

derrell · 2023-06-01T19:27:57Z

Yes, deep debugging requires a bit of getting used to, but once you get used to it, you learn to quickly run-until-function-exit on some of the calls. It's not that each is wrapped in a function; rather, the proxy wraps the entire object. Every access to the object runs through the proxy. That includes access to methods, so first, you'll see a get of the method itself, and then you'll see the method invocation. That fist get of the method is one that you learn to ignore and immediately step out of. As I worked with this system, I found it far, far easier to debug properties than the original generated code, once I got used to working with it and learned to quickly step past the non-property accesses.

derrell · 2023-06-01T19:31:10Z

BTW, the vast majority of those object properties already existed in the old property system, but you typically had no reason to notice them. I added a few as necessary features of the new property system.

johnspackman · 2023-06-01T19:49:51Z

Yes, deep debugging requires a bit of getting used to, but once you get used to it, you learn to quickly run-until-function-exit on some of the calls

Every other debugging experience I can think of just steps into the function. It looks really odd, and I can't imagine new users taking to it very easily.

BTW, the vast majority of those object properties already existed in the old property system

I don't remember seeing them; they were presumably in the prototype or in a closure before? My debugger now shows 210 properties when there were approx 30 before.

IMHO that is confusing and makes it difficult to see whats going on.

johnspackman · 2023-06-01T19:53:28Z

also there seems to be a problem, maybe connected to the function being an async function - I cannot step into my function, the value variable is the function, not the result of the function:

this is in the handler:

this is the actual function definition:

johnspackman

This PR has quite an impact on debuggability - every single method call (not just properties) now goes via the proxy handler code; IMHO that's a bit confusing and distracting. Also, every property now causes 7 fields to be added to each object, which adds pages of irrelevant details to an object.

AIUI Object.defineProperty can be used to achieve the desired effect (ie native property values) without using Proxy, meaning that debugging is much simpler.

Here's a proof of concept that native properties can be added to the existing v7 qx.Class classes:

https://tinyurl.com/2gk2jpvy

johnspackman

Pseudo properties need to be supported also, eg qx.ui.form.List has a selection property but it only appears as getSelection and not as selection:

derrell · 2023-06-02T12:38:39Z

I don't know what a pseudo property is. Something is either defined in the properties configuration, or it is not. There are a few places that kind of simulate properties, but I'm not sure what you'd have the code do here. Guess that when there is a getX method, that means there should be an x property automatically added? That doesn't work if the getter method is actually retrieving the value from someplace else, e.g., some sub-object as is commonly done. I don't think I'm comfortable with guessing...

Add cross compiler to 8.x beta

johnspackman · 2023-06-02T13:08:42Z

I don't know what a pseudo property is.

we discussed it here: #10410

There are a few places that kind of simulate properties

they do this because the property system did not allow custom storage

I don't think I'm comfortable with guessing...

It's what qooxdoo does and how binding works. It sniffs, basically deciding that if there is a get, set, reset, and change event, it is a property.

derrell · 2023-06-02T23:06:01Z

Ok, I've added support for pseudo-properties. I excluded the test for the reset method, as it seems it's not required: the example of MModelSelection's selected pseudo-property doesn't have it, so I'm assuming it's not a required component of pseudo-properties.

I also added tests for it, as there apparently weren't any previously.

johnspackman · 2023-06-05T10:03:15Z

source/class/qx/Class.js

+            // Next  try to parse the check string as JSDoc
+            let bJSDocParsed = false;
+            try {
+              const { parse } = require("jsdoctypeparser");


I appreciate that this is currently switched off, but IMHO we should not plan on including require in browser based builds because it forces the user to use npm and have a package; years ago, one of the 1&1 team added grunt and it was very frustrating to have to create a package.json etc just to be able to compile.

Remember that require in a browser-based app includes the browserified code. There should be o need for the user to use npm and have a package; the compiler should be browserifying that code from qooxdoo's node_modules.

That said, I don't have any strong need for this feature. We could strip it out completely. We could, later, implement it with our own code to parse JSDoc comments. Or whatever. Let me know if you'd like for me to entirely remove that code.

I think that the jsdoc parser would be useful, but at the moment browserify does not work as you're propising (although I think it would be good if it did). I havn't specifically tested, but the compiler was complaining that it could not find the module

It's possible that the node_modules path of qooxdoo needs to be added to the compiler's search for required modules for it to work as desired. That's a separate issue that I can look into it, but it's separate from this PR.

I think that the jsdoc parser would be useful, but at the moment browserify does not work as you're propising (although I think it would be good if it did). I havn't specifically tested, but the compiler was complaining that it could not find the module

I just pushed a fix to this. The compiler now looks in both the app's node_modules directory and qooxdoo's node_modules directory for browserifyable modules. This means that jsdoctypeparser, used by Class.js (when enabled by the environment variable, later), is found regardless of whether there is a node_modules directory in the user's app.

CHANGELOG.md

johnspackman · 2023-06-05T10:07:48Z

docs/core/classes.md

+```
+instance[2] = 42;
+value = instance[2];
+delete instance[2];


please can you explain how delete is implemented? EG does this mean that you could have two objects of the same class, but one has undefined for the property because it has been deleted but the other does not, even if the definition of the property says that an uninitialised value is impossible?

Calling delete instance[2] is identical to calling instance.removeAt(2) because the delete is trapped and removeAt is actually called in its stead.

johnspackman · 2023-06-05T10:13:43Z

source/class/qx/data/Array.js

+    },
+
+    delete(property) {
+      // If the property is a number or string representing a number...


delete dataArray[2] is not the same as dataArray.removeAt(2) because delete will simply create a hole in the array where as removeAt will shuffle the elements down. Does qx.data.Array support sparse arrays?

delete dataArray[2] in qx.data.Array is implemented as dataArray.removeAt(2). I don't believe that qx.data.Array supports sparse arrays, so we probably don't want dataArray.removeAt(2) to leave an undefined hole. The implementation is trivially changeable, though. Do you have suggestions for a different implementation? (It's marked as experimental, so what we do now can be changed later.)

But it's different semantics - calling delete myNativeArray[2] is not the same as myDataArray.removeAt(2)

Understood. What would you have it do instead?

throw an exception?

Ok. It now throws an exception by default. If environment variable qx.data.Array.deleteAsRemoveAt is true then it acts as I had implemented it originally.

…ass"

….deleteAsRemoveAt' is true

…uire()d modules

johnspackman · 2023-11-16T21:55:43Z

@derrell would it be possible to progress this PR? I appreciate you're busy, but it would be good to see it come to a conclusion

ChristianMayer · 2024-05-01T11:19:14Z

There wasn't any action for nearly a year now. Is this still active?

johnspackman · 2024-05-01T11:25:22Z

There wasn't any action for nearly a year now. Is this still active?

Yes - I have a version which seems to be complete but I need to merge the current master into it so that I can do the tests; I'm just stupidly busy at work at the moment, sorry for the delay - I'm really keen to get it out

derrell added 30 commits June 27, 2022 11:22

Add tests for missing member variable required by interface

4d680b7

Fix interface check for missing member variable

f7d4ca2

fix related tests

1ff9467

fix related tests

539ff50

Initial integration of new class/property system. Not yet functional.

581b4f4

Early test shows integration is now working. Much more testing needed...

926b94a

wrap constructor in case it was declared using "method" syntax

7e917ac

Fix instance and interface checks

bf6d96d

wip on integration bug fixes

6e8be8d

wip on integration bug fixes

453b3c3

wip: adding event and refresh handling.

a39a51c

add member declarations for previously undeclared members

a185254

wip on integration bug fixes

6826d6f

bug fixes during integration (building compiler)

08ebf43

bug fixes during integration (getting testtapper to run initially)

5e21aba

bug fixes during integration (getting testtapper to run initially)

16e8a57

bug fixes during integration (running testtapper tests)

f80ffc7

shrinkwrap updates

e3a19f6

more bug fixes towards getting test suite to pass

5123ca9

more bug fixes towards getting test suite to pass

6e94a9b

wip getting tests to pass

782e2e3

wip getting tests to pass

df0b6b9

wip getting tests to pass.

8f01616

wip getting tests to pass.

b0acf4b

wip getting tests to pass.

c243515

wip getting tests to pass.

1bb1f0c

wip getting tests to pass.

a47680f

wip getting tests to pass.

becb18d

wip getting tests to pass.

f1c6754

wip getting tests to pass.

672f76a

derrell and others added 2 commits May 29, 2023 14:24

properly resolve merge conflict

99d7d68

Merge branch 'master' into 8_0_beta

d816367

goldim reviewed May 31, 2023

View reviewed changes

source/class/qx/core/propertystorage/ImmutableDataArray.js Show resolved Hide resolved

derrell added 2 commits May 31, 2023 19:42

fix documentation

5f83547

Merge branch '8_0_beta' of github.com:qooxdoo/qooxdoo into 8_0_beta

c3be1c9

cboulanger approved these changes Jun 1, 2023

View reviewed changes

johnspackman added 2 commits June 1, 2023 12:56

docs and fixes.

5649a27

add db cache and support cleaning

d9db91c

johnspackman requested changes Jun 1, 2023

View reviewed changes

johnspackman requested changes Jun 2, 2023

View reviewed changes

Merge pull request #10601 from johnspackman/8_0_beta

4d15890

Add cross compiler to 8.x beta

add support and tests for pseudo-properties

65e18f9

johnspackman requested changes Jun 5, 2023

View reviewed changes

derrell added 5 commits June 5, 2023 09:13

fix typos

8b0ce64

reword docu: refer to new properties as "native" instead of "first-cl…

a19c7f4

…ass"

qx.data.Array 'delete' throws error unless environment 'qx.data.Array…

422e2e4

….deleteAsRemoveAt' is true

test 'qx.data.Array.deleteAsRemoveAt' in both configurations

95f1516

search qooxdoo node_modules in addition to app's node_modules for req…

1336610

…uire()d modules

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8 0 beta #10592

8 0 beta #10592

derrell commented May 13, 2023

cboulanger left a comment

johnspackman commented Jun 1, 2023

derrell commented Jun 1, 2023

derrell commented Jun 1, 2023

johnspackman commented Jun 1, 2023

johnspackman commented Jun 1, 2023 •

edited

Loading

johnspackman left a comment

johnspackman left a comment

derrell commented Jun 2, 2023

johnspackman commented Jun 2, 2023

derrell commented Jun 2, 2023

johnspackman Jun 5, 2023

derrell Jun 5, 2023

johnspackman Jun 5, 2023

derrell Jun 5, 2023

derrell Jun 7, 2023

johnspackman Jun 5, 2023

derrell Jun 5, 2023

johnspackman Jun 5, 2023

derrell Jun 5, 2023

johnspackman Jun 5, 2023

derrell Jun 5, 2023

johnspackman Jun 6, 2023

derrell Jun 7, 2023

johnspackman commented Nov 16, 2023

ChristianMayer commented May 1, 2024

johnspackman commented May 1, 2024

8 0 beta #10592

Are you sure you want to change the base?

8 0 beta #10592

Conversation

derrell commented May 13, 2023

cboulanger left a comment

Choose a reason for hiding this comment

johnspackman commented Jun 1, 2023

derrell commented Jun 1, 2023

derrell commented Jun 1, 2023

johnspackman commented Jun 1, 2023

johnspackman commented Jun 1, 2023 • edited Loading

johnspackman left a comment

Choose a reason for hiding this comment

johnspackman left a comment

Choose a reason for hiding this comment

derrell commented Jun 2, 2023

johnspackman commented Jun 2, 2023

derrell commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnspackman commented Nov 16, 2023

ChristianMayer commented May 1, 2024

johnspackman commented May 1, 2024

johnspackman commented Jun 1, 2023 •

edited

Loading