Tracked storage implementations #161

NullVoxPopuli · 2021-09-11T19:10:00Z

This PR requires: #160 be merged first

requires Ember 3.23+
upgrades to tracked-maps-and-sets@v3
drops support for Node 8 (wasn't tested against anyway)
deletes legacy implementation / code

NullVoxPopuli · 2021-09-11T19:24:39Z

addon/-private/array.ts

-    return createArrayProxy(arr.slice());
+    let clone = arr.slice();
+    // eslint-disable-next-line @typescript-eslint/no-this-alias
+    let self = this;


I'm using this blast from the past because not all of the proxy hooks have a receiver? or at least my editor didn't say that they did.

If I'm wrong, I'll gladly swap out all the self references to receiver.

Well, for the array case they all do - because the only hooks needed are get and set. So there you can use receiver. But for the object case you're right.

Actually no, you can't use receiver as it's the Proxy object, not the instance of the class that has the needed methods.

that's why let self = this 🙃

NullVoxPopuli · 2021-09-11T19:30:24Z

@chriskrycho / @pzuraq these are the failing tests atm, but do these failing tests imply that we need to fix something in tracked-maps-and-sets?

once resolved, I think this is ready to go?

boris-petrov · 2021-09-13T11:01:13Z

@NullVoxPopuli - are you sure tracked-maps-and-sets is needed at all after the changes in this PR?

NullVoxPopuli · 2021-09-13T11:03:08Z

Yeah, this library re-exports maps and sets, and uses them in the decorator

chriskrycho · 2021-09-13T13:56:12Z

I will try to dig into this PR tomorrow (very full day today!), but at a first pass without even having looked at it, I'd say:

Yeah, we definitely want those to pass!
Therefore, we should check and make sure they pass upstream in tracked-maps-and-sets – I think we have coverage of this same behavior there, but don't recall 100% off the top of my head.
If they do pass upstream and don't pass here, it means the decorator implementation is doing the wrong thing.

NullVoxPopuli · 2021-09-13T16:55:44Z

Excellent :D
Fix incorrect prototype chain to make instanceof work tracked-maps-and-sets#137
they do not pass. :D which is great, because it means I didn't do anything totally dumb while mindlessly doing this PR 🙃

boris-petrov · 2021-10-04T18:43:09Z

addon/-private/array.ts

+        let index = convertToInt(prop);
+
+        if (index !== null) {
+          self.storageFor(index);


@NullVoxPopuli - shouldn't this be self.readStorageFor(index) (like in tracked-object)? And readStorageFor should also be changed to getValue(storage) instead of return storage. Or am I missing something?

it's been a minute, but I think it's just a method naming inconsistency?

Nope, check the similar method in object.ts. It does getValue(storage) (which in the array case it doesn't do).

it does for the collection reference

boris-petrov · 2021-10-05T13:57:18Z

Now the implementation looks correct, thanks! A couple of notes:

instead of using // @private in object.ts you could do the same as in array.ts - use the private keyword.
storages = new Map(); in object.ts could be updated to match array.ts - i.e. have a better type-signature.
it's a pity that the tests didn't catch the lack of getValue(storage); in readStorageFor. If you have the time, you might add some to catch that case.

P.S.

oh, and the linting doesn't pass. 😄

NullVoxPopuli · 2021-10-06T15:25:18Z

I'll take a look as soon as tracked-maps-and-sets is released as that is a dependency for this work to move forward

chriskrycho · 2021-10-25T23:17:15Z

@NullVoxPopuli I've released tracked-maps-and-sets v3.0.2, which should unblock this! You'll need to basically run CI locally (or on your own fork) at present, though, as there's something blocking GH Actions on the newly-created org here (I've filed a ticket with GH support, but it's unclear how long it will take to get resolved). Note that this is also a prerequisite for #173. 😩 Lots of pieces in flight here. Let me know when you've rebased and I'll review!

chriskrycho · 2021-10-25T23:17:53Z

@NullVoxPopuli @SergeAstapov if need be I can basically fold these two PRs together.

… assertions from each other

NullVoxPopuli · 2021-10-26T00:20:49Z

Locally,

all pass -- yarn lint
- template
- eslint
- tsc
mixed passing -- yarn test
- standardo
- try scenarios
  - passing:
    - release
    - 3.24
  - failing (epected) (needs ember-auto-import@v2 in devDeps):
    - canary
    - beta
  - failed, needs investigation - not supported? 3.20 support will be dropped (by ember) when 3.28 becomes LTS, yeah?
    - 3.20 fails the {{each}} / {{each-in}} tests
yarn prepublishOnly (because of the secret incantation -- it is not the same as lint:tsc`)

chriskrycho

Overall 👍🏻 – bunch of tweaks around privacy, but that's it!

README.md

chriskrycho · 2021-10-26T15:33:06Z

README.md

-* Ember CLI v2.13 or above
-* Node.js v8 or above
+* Ember.js v3.23 or above
+* Ember CLI v3.23 or above


Does it actually require a CLI bump? I'd prefer to just leave CLI alone unless there's a reason to change it.

probably not!

but, we aren't testing against ember-cli 2.13

README.md

addon/-private/array.ts

addon/-private/object.js

addon/-private/array.ts

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

…onversion

addon/-private/array.ts

chriskrycho

Ran CI locally:

------ RESULTS ------

Scenario ember-lts-3.24: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ 3.24.3             │ 3.24.3                       │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.28: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.28.0            │ 3.28.4                       │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-release: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ https://s3.amazon… │ 3.28.4-release+62976fe7      │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-beta: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ https://s3.amazon… │ 4.0.0-beta.5.beta+829c1131   │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-canary: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ https://s3.amazon… │ 4.1.0-alpha.2.canary+67d465… │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘


2 scenarios failed
3 scenarios succeeded
5 scenarios run

This is as expected.

NullVoxPopuli force-pushed the tracked-storage branch from 95d437f to 51f78de Compare September 11, 2021 19:23

NullVoxPopuli commented Sep 11, 2021

View reviewed changes

chriskrycho added breaking enhancement New feature or request labels Sep 14, 2021

boris-petrov reviewed Oct 4, 2021

View reviewed changes

NullVoxPopuli force-pushed the tracked-storage branch from 3d9b55e to 7e7989b Compare October 5, 2021 11:56

chriskrycho mentioned this pull request Oct 25, 2021

Support Ember 4 and drop Ember <3.24 TLS #173

Merged

NullVoxPopuli added 4 commits October 25, 2021 20:08

Rework TrackedObject to use the tracked storage primitives

463e969

Rework TrackedArray to use the tracked storage primitives

db6d69c

decorator-test.js: add assertion messages so we can differentiate the…

a1e79ff

… assertions from each other

Use consistent naming with the other tracked storage implementations

e16c492

NullVoxPopuli force-pushed the tracked-storage branch from 7e7989b to e16c492 Compare October 26, 2021 00:09

NullVoxPopuli marked this pull request as ready for review October 26, 2021 00:09

NullVoxPopuli added 2 commits October 25, 2021 20:10

Upgrade to latest tracked-maps-and-sets

de6a07c

Resolve lint issues

66af90e

Update ember-try config and CI

7a45111

chriskrycho requested changes Oct 26, 2021

View reviewed changes

NullVoxPopuli and others added 3 commits October 26, 2021 12:11

Update README.md to specify that Node 12 is minimum required

ccb6753

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

Update README.md to specify that 3.24 LTS is min supported ember-source

fea805d

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

rename key to index in readStorageFor

5edc87e

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

NullVoxPopuli and others added 5 commits October 26, 2021 12:12

rename key to index in dirtyStorageFor

924bfa8

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

Update addon/-private/object.js

504231d

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

Update addon/-private/array.ts

85fa6f3

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

Update addon/-private/array.ts

4f7c44d

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

Update browserlist db, update ember-cli-babel, finish private field c…

a14590d

…onversion

chriskrycho reviewed Oct 26, 2021

View reviewed changes

addon/-private/array.ts Outdated Show resolved Hide resolved

Improve TS idioms

abe972f

chriskrycho approved these changes Oct 26, 2021

View reviewed changes

chriskrycho merged commit 65b1b79 into tracked-tools:master Oct 26, 2021

chriskrycho mentioned this pull request Oct 26, 2021

Improve Object Proxy #112

Closed

NullVoxPopuli mentioned this pull request Nov 8, 2021

Uncaught Error: Could not find module tracked-maps-and-sets/-private/util imported from (require) NullVoxPopuli/ember-deep-tracked#211

Closed

chriskrycho mentioned this pull request Dec 14, 2021

Using new TrackedArray<string>() in TypeScript #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracked storage implementations #161

Tracked storage implementations #161

NullVoxPopuli commented Sep 11, 2021 •

edited

Loading

NullVoxPopuli Sep 11, 2021

boris-petrov Oct 4, 2021

boris-petrov Oct 4, 2021

NullVoxPopuli Oct 5, 2021

NullVoxPopuli commented Sep 11, 2021

boris-petrov commented Sep 13, 2021

NullVoxPopuli commented Sep 13, 2021

chriskrycho commented Sep 13, 2021

NullVoxPopuli commented Sep 13, 2021

boris-petrov Oct 4, 2021

NullVoxPopuli Oct 4, 2021

boris-petrov Oct 4, 2021

NullVoxPopuli Oct 5, 2021

boris-petrov commented Oct 5, 2021 •

edited

Loading

NullVoxPopuli commented Oct 6, 2021

chriskrycho commented Oct 25, 2021

chriskrycho commented Oct 25, 2021

NullVoxPopuli commented Oct 26, 2021

chriskrycho left a comment

chriskrycho Oct 26, 2021

NullVoxPopuli Oct 26, 2021

NullVoxPopuli Oct 26, 2021

chriskrycho left a comment

Tracked storage implementations #161

Tracked storage implementations #161

Conversation

NullVoxPopuli commented Sep 11, 2021 • edited Loading

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

NullVoxPopuli commented Sep 11, 2021

boris-petrov commented Sep 13, 2021

NullVoxPopuli commented Sep 13, 2021

chriskrycho commented Sep 13, 2021

NullVoxPopuli commented Sep 13, 2021

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

boris-petrov commented Oct 5, 2021 • edited Loading

NullVoxPopuli commented Oct 6, 2021

chriskrycho commented Oct 25, 2021

chriskrycho commented Oct 25, 2021

NullVoxPopuli commented Oct 26, 2021

chriskrycho left a 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

chriskrycho left a comment

Choose a reason for hiding this comment

NullVoxPopuli commented Sep 11, 2021 •

edited

Loading

boris-petrov commented Oct 5, 2021 •

edited

Loading