Convert to typescript #9

usu · 2020-11-27T06:41:22Z

I've started to dive into Typescript. Looks promising to me so far.

@carlobeltrame Can you have a look at it if you feel this goes into the right direction? If so, I'd continue piece-by-piece with the rest of the code.

carlobeltrame · 2020-11-28T16:11:48Z

Ah wow, nice, thanks! Maybe have a look at https://github.com/ecamp/hal-json-vuex/tree/refactor-to-classes where I started to refactor the storeValueProxy file into classes. I think that should be completed before moving to Typescript, what do you think?

usu · 2020-11-29T20:44:42Z

I think that should be completed before moving to Typescript, what do you think?

Ah nice, didn't see that branch before. Yes, makes sense to complete & merge that one before. Otherwise we'll have tons of conflicts.

…nterface definitions

carlobeltrame

Looking good :) a few comments from my side for now. Let me know if you want me to continue this.

src/StoreValue.ts

src/QueryablePromise.ts

src/interfaces/ApiActions.ts

src/interfaces/Config.ts

webpack.config.js

src/interfaces/StoreData.ts

usu · 2020-12-06T11:54:14Z

@carlobeltrame I believe this is in a state where you could have another look at it. Would say 95% complete as a first shot. Appreciate a careful review.

Some open actions (potentially as separate issues, to be discussed):

Discuss and resolve the various inline comments marked as "TODO"
Avoid usage of "any" and inline rule exclusions
Real world test in ecamp3 (only tested independetly so far)
Consider splitting up index (Install/Export, API actions, helper functions)
Consider implementation of CanHaveItems as mixin/composable instead of as a base class
Naming and file organization: Names of classes/interfaces/etc. have now grown historically. I think it makes sense to take a step back and review what the best and most consistent naming would be.
LoadingStoreCollection is a class with a static function only which is a Javascript antipattern. Consider implementing as simple function or integrate back to LoadingStoreValue.
Unit tests for separate components?

carlobeltrame

Nice work, thank you! I have tried to answer all your todos. There is still some work to do before we can merge this I think, but if you feel that some things should be done in later iterations let me know.

src/CanHaveItems.ts

src/LoadingStoreValue.ts

src/CanHaveItems.ts

carlobeltrame · 2020-12-06T13:19:05Z

src/LoadingStoreCollection.ts

+   * @param arrayLoaded     Promise that resolves once the array has finished loading
+   * @param existingContent optionally set the elements that are already known, for random access
+   */
+  static create (loadArray: Promise<Array<Resource> | undefined>, existingContent: Array<Resource> = []): Array<Resource> {


LoadingStoreCollection is a class with a static function only which is a Javascript antipattern. Consider implementing as simple function or integrate back to LoadingStoreValue.

The create method should probably be replaced with a constructor, right? If that's not possible in TypeScript, we could just store the existingContent on the LoadingStoreCollection instance and wrap it that way.

In this case, we either need to wrap it. Or extend the Array class. I tried latter, and somehow failed. Let my try former 😵

One conclusion from my experiments: Extending the Array class is not possible, because the LoadingStoreCollection has different type signatures for either find or map:

If LoadingStoreCollection inherits from Array<Resource> or some other Array<Something>, or we make a generic LoadingStoreCollection<T>, the find method can no longer return a LoadingStoreValue, but must return Resource or Something or T.

If LoadingStoreCollection inherits from Array<LoadingStoreValue>, the map method cannot be made to work the way we want, because it has to potentially transform the LoadingStoreValues into objects of other classes, depending on the callback passed to map (the callback might map every entry to an integer, for example).

So a wrapper that implements Resource or better Collection is the only option I see to avoid the static create method.

TODO: Try something like LoadingStoreValue<T> extends T, this way the Array contract could maybe hold

src/LoadingStoreValue.ts

src/QueryablePromise.ts

src/StoreValue.ts

usu · 2020-12-06T17:24:09Z

There is still some work to do before we can merge this I think, but if you feel that some things should be done in later iterations let me know.

Yes agree. Let me see how far I get with the open points. Some of the questions I raised in the earlier comment might be too big to also squeeze into this pull request.

By the way: I also just uploaded a rebased version of the code status as of today (separating renaming of files and actual code change). Might be a bit easier to verify the code changes by looking at the last commit only:
usu@17d57ff

src/HasItems.ts

carlobeltrame · 2021-03-23T15:07:28Z

I have also continued this a little, and I have come to the conclusion that for HasItems, normal inheritance should be enough, we don't need the more generic variant of mixins you described (a mixin is valuable if you want to apply it to multiple different base classes, but we only ever apply it to StoreValue).
But I am not quite finished with resolving all type-related refactorings I was doing. Maybe we can have a look at it some time.

carlobeltrame · 2021-03-23T21:40:31Z

Mein aktuellster Stand ist jetzt auf diesem Branch, damit wir uns nicht mehr über-pushen: https://github.com/ecamp/hal-json-vuex/tree/feature/typescript

src/HasItems.ts

src/index.ts

usu · 2021-04-05T11:48:55Z

@carlobeltrame
The 2 open points discussed last week are now implemented

$loadItems resolves to the collection object instead of the array of items
get cannot be used anymore with embedded collections (limited to reloading)

We'll probably still see more changes on the embedded collections. I don't think it's coded super cleanly.

But would appreciate if we can merge the current status and then divide future work into smaller chunks.

Alternatively, we could also open a v2-alpha branch (+v2 alpha releases) and start PR/merging into the alpha branch until we feel the interface is robust and stable enough.

carlobeltrame

LGTM, thanks a lot!

The alpha strategy sounds good. In case we ever decide to backport anything to an old major version, I'd rather have a sound pre-typescript version than an early typescript version which may contain flaws that we do not know as of now.

carlobeltrame · 2021-04-06T20:31:42Z

src/EmbeddedCollection.ts

@@ -0,0 +1,48 @@
+// import LoadingStoreCollection from './LoadingStoreCollection'


carlobeltrame · 2021-04-06T20:33:02Z

src/EmbeddedCollection.ts

+   * @param reloadProperty  property in the containing entity under which the embedded collection is saved
+   * @param apiActions      dependency injection of API actions
+   * @param config          dependency injection of config object
+   * @param loadParent      a promise that will resolve when the parent entity has finished (re-)loading


Outdated jsDoc comment

carlobeltrame · 2021-04-06T20:35:13Z

src/HasItems.ts

+     * Returns true if any of the items within 'array' is not yet known to the API (meaning it has never been loaded)
+     */


Indentation

carlobeltrame · 2021-04-06T20:39:57Z

src/HasItems.ts

+
+      // eager loading of 'fetchAllUri' (e.g. parent for embedded collections)
+      if (config.avoidNPlusOneRequests && reloadUri) {
+        return apiActions.reload({ _meta: { reload: { uri: reloadUri || '', property: reloadProperty || '' } } }) as Promise<Collection>


This as is here to convince TypeScript that the server has previously sent a collection, so it will send a collection again, right? Consider adding a comment that explains this

usu · 2021-04-09T17:04:25Z

@carlobeltrame I've changed the target branch to next instead of master. So we merge and then tag/publish as 2.0.0-alpha.0? Or 1.3.0-alpha.0?

carlobeltrame · 2021-04-11T13:04:58Z

Yes, +1 for 2.0.0-alpha.0, since technically we already made some small breaking changes (to undocumented features), and this allows us to clean up some more if we deem it necessary.

carlobeltrame · 2021-04-11T18:47:38Z

v2.0.0-alpha.0 is published: https://www.npmjs.com/package/hal-json-vuex?activeTab=versions

usu added 2 commits November 26, 2020 21:52

initial Typescript config

af87dfa

extract ServerException & fix eslint & fix tests

f914c86

usu added 2 commits December 1, 2020 18:41

merge master into feature/typescript

8a578e4

Typescript conversion for StoreValue & QueryablePromise + necessary i…

3bc1133

…nterface definitions

usu marked this pull request as draft December 1, 2020 20:53

carlobeltrame requested changes Dec 2, 2020

View reviewed changes

usu added 11 commits December 5, 2020 08:57

require QueryablePromise consistently; override .then/.catch

f69286a

refactor QueryablePromise

81435c1

convert LoadingStoreValue to Typescript

d585175

items/allItems as getters

a34b474

convert LoadingStoreCollection to TypeScript

9ae112b

convert EmbeddedCollection and CanHaveItems to Typescript

1348588

items and allItems as normal getters

9477b05

convert normalizeUri

6a56036

convert storeModule

0707c5f

convert index to Typescript (partial; WIP)

522d8ae

convert index to Typescript (finalized)

e3698e8

usu marked this pull request as ready for review December 6, 2020 11:54

carlobeltrame requested changes Dec 6, 2020

View reviewed changes

usu added 7 commits December 7, 2020 19:01

bugfixes: get() empty root; no return value from preparePostData

42b200e

include DOM libs

709ba69

fix toString()

34c4bf0

fix docu + lint warnings

fbfe860

simplifications in CanHaveItems

23cd7b5

avoid unnecessary usage of QueryablePromise

8365df6

use _meta.reloading instead of QueryablePromise

48bc1b9

usu commented Mar 23, 2021

View reviewed changes

src/HasItems.ts Outdated Show resolved Hide resolved

fix linter

56a6a23

usu added 2 commits March 23, 2021 17:05

EmbeddedCollection with HasItems (remove legacy CanHaveItems)

6fc848d

fix loadItems

93682f4

usu force-pushed the feature/typescript branch from 1db66dc to 93682f4 Compare March 23, 2021 18:28

renovate-bot added 3 commits March 24, 2021 13:35

Update dependency webpack to v5.28.0

459437f

Update dependency eslint to v7.23.0

bc4fd7a

Update dependency webpack-cli to v4.6.0

d0559e3

usu commented Mar 30, 2021

View reviewed changes

src/HasItems.ts Outdated Show resolved Hide resolved

usu commented Mar 30, 2021

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

usu commented Mar 30, 2021

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

usu and others added 8 commits March 30, 2021 21:41

fix various TODOs

0cd1f8e

Update dependency webpack to v5.29.0

5c63788

Update dependency webpack to v5.30.0

4a94028

loadItems returns collection object (not array of items anymore)

accb52f

prohibit get for embedded collections (only reload allowed)

40948f9

merge master into feature/typescript

f77c0ac

fix TS compiler errors

71859b6

EmbeddedCollectionType --> EmbeddedCollectionMeta

67b9cd0

usu requested a review from carlobeltrame April 5, 2021 11:42

carlobeltrame approved these changes Apr 6, 2021

View reviewed changes

usu changed the base branch from master to next April 9, 2021 16:44

inline doc improvements

54e59fb

carlobeltrame merged commit 406bdc1 into ecamp:next Apr 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to typescript #9

Convert to typescript #9

usu commented Nov 27, 2020

carlobeltrame commented Nov 28, 2020

usu commented Nov 29, 2020 •

edited by manuelmeister

Loading

carlobeltrame left a comment

usu commented Dec 6, 2020 •

edited

Loading

carlobeltrame left a comment

carlobeltrame Dec 6, 2020

usu Dec 7, 2020

carlobeltrame Mar 23, 2021 •

edited

Loading

carlobeltrame Mar 30, 2021

usu commented Dec 6, 2020

carlobeltrame commented Mar 23, 2021

carlobeltrame commented Mar 23, 2021 •

edited

Loading

usu commented Apr 5, 2021

carlobeltrame left a comment

carlobeltrame Apr 6, 2021

carlobeltrame Apr 6, 2021

carlobeltrame Apr 6, 2021

carlobeltrame Apr 6, 2021

usu commented Apr 9, 2021

carlobeltrame commented Apr 11, 2021

carlobeltrame commented Apr 11, 2021

		@@ -0,0 +1,48 @@
		// import LoadingStoreCollection from './LoadingStoreCollection'

		* Returns true if any of the items within 'array' is not yet known to the API (meaning it has never been loaded)
		*/

Convert to typescript #9

Convert to typescript #9

Conversation

usu commented Nov 27, 2020

carlobeltrame commented Nov 28, 2020

usu commented Nov 29, 2020 • edited by manuelmeister Loading

carlobeltrame left a comment

Choose a reason for hiding this comment

usu commented Dec 6, 2020 • edited Loading

carlobeltrame left a comment

Choose a reason for hiding this comment

carlobeltrame Dec 6, 2020

Choose a reason for hiding this comment

usu Dec 7, 2020

Choose a reason for hiding this comment

carlobeltrame Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

carlobeltrame Mar 30, 2021

Choose a reason for hiding this comment

usu commented Dec 6, 2020

carlobeltrame commented Mar 23, 2021

carlobeltrame commented Mar 23, 2021 • edited Loading

usu commented Apr 5, 2021

carlobeltrame left a comment

Choose a reason for hiding this comment

carlobeltrame Apr 6, 2021

Choose a reason for hiding this comment

carlobeltrame Apr 6, 2021

Choose a reason for hiding this comment

carlobeltrame Apr 6, 2021

Choose a reason for hiding this comment

carlobeltrame Apr 6, 2021

Choose a reason for hiding this comment

usu commented Apr 9, 2021

carlobeltrame commented Apr 11, 2021

carlobeltrame commented Apr 11, 2021

usu commented Nov 29, 2020 •

edited by manuelmeister

Loading

usu commented Dec 6, 2020 •

edited

Loading

carlobeltrame Mar 23, 2021 •

edited

Loading

carlobeltrame commented Mar 23, 2021 •

edited

Loading