Refactor front end `Meta` class and a lot of surrounding code. #1109

seancolsen · 2022-03-01T16:40:03Z

Context

This is laying groundwork for moving the front end to use column ids instead of column names, as required by #1047

Goals

Loosen the coupling between: (A): the functionality which saves tab data to the URL; and (B): the underlying data structures used within the tabs system and the tabular data. Previously, we were passing around MetaParams arrays like this

export type MetaParams = [
  number[], // [pageSize, page]
  string[], // [sortcolumn, sortorder:'a'|'d', sc, so ...]
  string[], // [groupcolumn, gc, gc]
  string[], // [filtercombination:'a'|'o', filtercolumn, condition, value ...]
];

which led to code like this, which is very hard to read

function getTabsFromConfig(
  schemaId: SchemaEntry['id'],
  tabListConfig?: TabListConfig,
): MathesarTab[] {
  const tabs: MathesarTab[] = [];
  const tableStoreData = get(getTablesStoreForSchema(schemaId));
  tabListConfig?.tabularDataParamList?.forEach((entry) => {
    const table = tableStoreData.data.get(entry[1]);
    if (table) {
      tabs.push({
        id: calculateTabularTabId(entry[0], entry[1]),
        label: table.name,
        isNew: false,
        tabularData: getTabularContent(entry[0], entry[1], entry),
      });
    }
  });
  return tabs;
}

(That getTabsFromConfig function doesn't really have an analog after my changes -- it's been exploded into many different functions.)

Splinter off some of the logic within Meta into separate classes for Pagination, Filtering, Sorting, and Grouping.
Move TabularType out of d.ts file. Jest would not run my tabDataSaver.test.ts file the TabularType value being in d.ts file.
Provide strong typing for the records API get request params.
Add SimpleSelect component which provides cleaner DX than the Select component. In Refactor Select and SimpleSelect components, merging them into one #1099 we'll merge the two components into one component called Select.
Remove unused type and parentId properties from Display and Meta classes.
Add base64 encoding to the serialized JSON tab data so that it doesn't require any URL encoding.

Caveats

Filtering UI is commented out because that will all be changing soon and is already broken in master.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the master branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

seancolsen · 2022-03-01T16:51:21Z

mathesar_ui/src/App.svelte

+  // reliably set the query params when the schema changes. It actually _will_
+  // set the query params _sometimes_, but we weren't able to figure out why the
+  // behavior is inconsistent.
+  beginUpdatingUrlWhenSchemaChanges(currentSchemaId);


@pavish This is how I'm handling that finicky routing issue we spent some time discussing yesterday.

I played with it for a while after our call. There's definitely something fishy going on which inconsistently causes the stores within TabList to update after tinro has performed client-side navigation. I tried to trace it down using the in-browser debugging tools but just ended up in a rat's nest of compiled Svelte code.

So this call to beginUpdatingUrlWhenSchemaChanges is my hacky work-around. I had to put it within a pretty top-level location in the app in order to avoid cyclical dependency linting errors. When/if we permit the user to switch databases, we'll need to implement a similar hack.

I'm interested in eventually moving Mathesar to SvelteKit (once it's stable), in which case we'll need do re-think all the routing anyway. So I'm okay with some hack in the mean time.

I'm interested in eventually moving Mathesar to SvelteKit (once it's stable), in which case we'll need do re-think all the routing anyway. So I'm okay with some hack in the mean time.

I estimate this mean time to be atleast a couple years. Even then I wouldn't want to switch to SvelteKit until it has an official python adapter. Currently, if we need to switch, we'll need a node js middle layer, which is just pointless for us. There is a SvelteKit static adapter but that'll essentially be the same (if not less performant) than directly using Svelte with pre-rendered data as we do now. So, I doubt we'd ever move to SvelteKit.

I'll take a look at this issue. We need to find the cause and fix it.

@seancolsen

I tracked down the "when" of the problem:

All the callbacks of subscribers of tabs and activeTab get called everytime the TabContainer component is destroyed and re-created. This causes recalculation and updating of the query params.

To test this, in Base.svelte, you can either remove binding tabs and activeTab to TabContainer, or remove the #if condition for tabs.length > 0 for rendering the TabContainer.

After this change, you can notice that the url does not get updated when schemas are switched (except when a new schema is opened which creates a new TabList instance, in which case it is expected). The url not updating is the behaviour we expect.

Regarding the "why" of the problem:

I still have no clue.

Callback of stores are usually called whenever there is a first-subscriber and then if any dependencies change. Here, there are multiple subscribers for both tabs and activeTab which are active, I checked if subscription count goes to 0 when schema changes but that doesn't seem to be the case. So we can eliminate the first-subscriber being a reason.

Why the callback gets updated when TabContainer is destroyed and re-created could either be a problem with how we utilize these stores or most probably a bug in Svelte itself.

We need an open issue for this and try to replicate it on the Svelte REPL. If it's a svelte issue, we need to raise it on the svelte repo.

The hack:

I don't see the hack you've implemented as a hack. It's valid code that is required. The router was never at fault, we are manipulating the query params for our own custom logic. We would need to something similar even if we use SvelteKit, or basically any router.

It seems to be an issue with Svelte, or maybe it was intended but I don't see the behaviour mentioned in the docs. I opened sveltejs/svelte#7330 for this.

@seancolsen I think we can remove the comment which says this is a hacky way of doing things and explain why we are calling the function here.

When we have the time, it would be better if we had a call and I could explain in detail on why query params are manipulated imperatively. I think it is essential that you understand the reasoning behind it and it would be best if we document it all down in our wiki.

There is a SvelteKit static adapter but that'll essentially the same (if not less perfomant) than directly using Svelte with pre-rendered data as we do now.

During our sync call, I'm also curious to explore this point above more, because that definitely does not match not my understanding.

I'd like to understand from you (if you're able to explain it) why the routing behavior is inconsistent

The routing behaviour is not inconsistent, it works exactly as it is intended. The router is not at fault here. The upstream issue from svelte caused our url update method to get called which modified the query params.

Can you explain what exactly you find inconsistent?

Can you explain what exactly you find inconsistent?

Before I added the call to beginUpdatingUrlWhenSchemaChanges, I was observing that sometimes Base.svelte would re-render its TabContainer child (causing the query params to update), and sometimes it would not. In the changes before this PR, it was about ~95% likely to re-render, which is why, most of the time, the query params seemed to update. But I absolutely observed it to not be updating sometimes. Then, with the changes in my PR, it was re-rendering maybe about ~5% of the time, which is what let me initially to believe that my PR had a bug which was not present in master. But (as we saw on our call, and as I later confirmed through more rigorous testing) the code in my PR would sometimes update the query params.

I suspect it has something to do with the fact that we're deciding whether to render TabContainer based on the value of $tabs, which is subject to the upstream bug. There may be some sort of race condition or inconsistent garbage collection or something there.

It has to do with TabContainer being created and destroyed, since it is wrapped within an #if block. The upstream bug calls all subscribers of tabs and activeTab stores, each time TabContainer is created.

Assume you opened a table in Schema 1, which creates the TabContainer and TabList instance, which inturn updates the url. Then you open Schema 2, which contains no tables, so TabContainer gets destroyed. Note, we create a new TabList instance when a schema is opened for the first time, so the url update will happen when Schema 2 is opened.

When we switch back to Schema 1, the TabContainer gets created again, which updates the url. If you then open a table in Schema 2, switching between schemas do not update the url.

I assume in your local environment, you mostly tested by opening tables in both schemas, so you only noticed the url update happening ~5% of the time. If you hadn't you'd notice it behaves just like it did in the previous master.

Interesting. Ok it sounds like there's a good chance I was not being observant enough of these small details while testing, and likely the behavior was consistent but my test scenarios were inconsistent. Thanks for explaining!

codecov-commenter · 2022-03-02T07:40:43Z

Codecov Report

Merging #1109 (6634937) into master (24cfafa) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1109   +/-   ##
=======================================
  Coverage   93.44%   93.44%           
=======================================
  Files         112      112           
  Lines        4209     4209           
=======================================
  Hits         3933     3933           
  Misses        276      276

Flag	Coverage Δ
pytest-backend	`93.44% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24cfafa...6634937. Read the comment docs.

pavish · 2022-03-03T13:07:23Z

mathesar_ui/src/component-library/common/utils/typeUtils.ts

@@ -0,0 +1,7 @@
+function hasLabelProperty(v: unknown): v is { label: unknown } {
+  return typeof v === 'object' && v !== null && 'label' in v;


We could probably go another step and make the 'label' key configurable. This will retain existing Select functionality as well as retain type safety. We can think about it in the upcoming Select improvements PR.

pavish · 2022-03-03T13:09:27Z

mathesar_ui/src/component-library/pagination/Pagination.svelte

+  // TODO: @seancolsen says:
+  // > Refactor `pageCount` to no longer be an exported prop. We're exporting it
+  // > just so the parent component can access the calculation done within this
+  // > component. That's an unconventional flow of data. I'd rather do the


It's unconventional for other frameworks but the whole point of the bind keyword is to do exactly this. There are even dedicated readonly bind properties that Svelte provides. I'd say this is one of the intended ways of doing things in Svelte.

the whole point of the bind keyword is to do exactly this

I disagree. I would say the point of bind is to allow data to flow from child to parent. bind:value on an input element allows data that originates in the input element (from user input) to flow up to the parent. That's great.

This Pagination component is different. The data for pageCount originates in the parent component as total and pageSize. The pageCount value is simply a derivation of those two values, which Pagination never modifies. Thus the source-of-truth data here is being passed down. Then a derivation of that data is being passed back up. That's weird an unconventional. We have lots of bind usage in our codebase which is fine because the source of truth for the data actually originates in the child. But this is the only one I've seen which, I would say, abuses the bind pattern. Grappling with this weirdness costed me time during this refactor, which is why I suggested we clean it up.

I see your point regarding bind keyword.

For the pageCount property however, I strongly feel that it should be the child component's property and not be passed down from the parent.

Even though pageSize and total are passed down, pageCount is still a property which is calculated within the child component inorder for it to work as expected. The source of truth for pageCount is where the calculation happens and not where it's dependencies are passed from.

We can remove the bind keyword here and dispatch an event when pageCount changes and use that to update any property on the parent.

We can remove the bind keyword here and dispatch an event when pageCount changes and use that to update any property on the parent.

That would be problematic in the same way.

If I were designing this from scratch, I'd make a PaginationStats object like this:

class PaginationStats { readonly recordCount: number; readonly pageSize: number; readonly pageCount: number; constructor({ recordCount, pageSize }: { recordCount: number, pageSize: number, }) { this.recordCount = recordCount; this.pageSize = pageSize; this.pageCount = Math.ceil(this.recordCount / this.pageSize); } }

Then I'd make a stats prop on Pagination which accepts an instance of PaginationStats.

In our code currently, the StatusPane needs access to the pageCount. The only way it can get the pageCount is to render a Pagination component. That's deeply wierd to me. What if we want to take Pagination out of StatusPane but keep pageCount in StatusPane? Since StatusPane needs access to pageCount, pageCount should not be coupled to Pagination. The design above decouples pageCount from Pagination.

That would be problematic in the same way.

Not necessarily. pageCount would be decoupled since the parent's pageCount and the child's pageCount are different variables.

What if we want to take Pagination out of StatusPane but keep pageCount in StatusPane

We'd pass down pageCount to StatusPane.

The only way it can get the pageCount is to render a Pagination component.

It's the only current way to do it. If we absolutely need to calculate pageCount on the parent, we'd best expose a util method in the module context of the Pagination component. That way any file can do:
import { calculatePageCount } from 'Pagination.svelte'

If I were designing this from scratch, I'd make a PaginationStats object like this
Then I'd make a stats prop on Pagination which accepts an instance of PaginationStats.

This is a design pattern I'd go with only when it is absolutely essential.

It makes the Pagination component reliant on the PaginationStats object, which means that anyone wanting to use the Pagination component needs to create an instance of the PaginationStats class. Reactive updates of page and pageSize would always need creating a new object, leading to boilerplate code wherever we use this. It's one of the annoying patterns/aspects of React that made me like Svelte.

We can solve this simply by exporting some util methods, as I mentioned above.

The utils pattern you suggest above is great too. It's certainly less code than my example. The calculation function would get called more times than in my proposal, but that's fine since it's not a perf concern here. I'll update my code comment to point to this conversation, and recommend the utils function approach.

pavish · 2022-03-03T13:15:28Z

mathesar_ui/src/utils/storeUtils.ts

+export function collapse<T>(outerStore: Readable<Readable<T>>): Readable<T> {
+  // This is memory-safe because the Unsubscriber function gets returned from
+  // the callback passed to `derive`.
+  //
+  // From https://svelte.dev/docs#run-time-svelte-store-derived
+  //
+  // > If you return a function from the callback, it will be called when a) the
+  // > callback runs again, or b) the last subscriber unsubscribes.
+  return derived(outerStore, (innerStore, set) => innerStore.subscribe(set));
+}
+
+function arrayWithValueSetAtIndex<T>(array: T[], index: number, item: T): T[] {
+  const result = [...array];
+  result[index] = item;
+  return result;
+}
+
+/**
+ * Unite an array of stores into a store of arrays.
+ */
+export function unite<T>(stores: Readable<T>[]): Readable<T[]> {
+  let results: T[] = [];
+  return readable(results, (set) => {
+    const unsubscribers = stores.map((store, index) =>
+      store.subscribe((value) => {
+        results = arrayWithValueSetAtIndex(results, index, value);
+        set(results);
+      }),
+    );
+    // This is memory safe because when the last subscriber unsubscribes from
+    // the `unite` store, the function below will ensure that we're
+    // unsubscribing from all the inner stores.
+    return () => unsubscribers.forEach((unsubscriber) => unsubscriber());
+  });
+}


I love these utility functions!

pavish

@seancolsen Nice work! I didn't test it thoroughly but I didn't see any issues. I had a few comments that could evoke discussions on patterns we can establish. There are no action items for changes in this PR.

seancolsen requested a review from a team March 1, 2022 16:40

github-actions bot requested review from dhruvkb, dmos62, ghislaineguerin, kgodey, mathemancer, pavish, silentninja and zackkrida March 1, 2022 16:40

Refactor Meta class and surrounding code

87cb10d

seancolsen force-pushed the refactor_ui_meta branch from e6bcfdb to 87cb10d Compare March 1, 2022 16:42

seancolsen commented Mar 1, 2022

View reviewed changes

pavish self-assigned this Mar 2, 2022

Merge branch 'master' into refactor_ui_meta

b7f38e8

seancolsen mentioned this pull request Mar 2, 2022

Filter/Sort/Group are broken when referencing a deleted column #987

Closed

seancolsen and others added 2 commits March 2, 2022 10:01

Merge branch 'master' into refactor_ui_meta

6634937

Merge branch 'master' into refactor_ui_meta

e27b223

pavish reviewed Mar 3, 2022

View reviewed changes

pavish approved these changes Mar 3, 2022

View reviewed changes

pavish merged commit b6a1b38 into master Mar 3, 2022

pavish deleted the refactor_ui_meta branch March 3, 2022 13:29

This was referenced Mar 3, 2022

Add code comments linking to discussions #1133

Merged

Change .d.ts files to .ts files #1135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor front end `Meta` class and a lot of surrounding code. #1109

Refactor front end `Meta` class and a lot of surrounding code. #1109

seancolsen commented Mar 1, 2022 •

edited

Loading

seancolsen Mar 1, 2022 •

edited

Loading

pavish Mar 2, 2022 •

edited

Loading

pavish Mar 2, 2022 •

edited

Loading

pavish Mar 2, 2022

pavish Mar 3, 2022

seancolsen Mar 3, 2022

pavish Mar 3, 2022

seancolsen Mar 3, 2022

pavish Mar 3, 2022 •

edited

Loading

seancolsen Mar 3, 2022

codecov-commenter commented Mar 2, 2022 •

edited

Loading

pavish Mar 3, 2022

pavish Mar 3, 2022

seancolsen Mar 3, 2022

pavish Mar 3, 2022

seancolsen Mar 3, 2022

pavish Mar 3, 2022 •

edited

Loading

seancolsen Mar 3, 2022

pavish Mar 3, 2022

pavish left a comment •

edited

Loading

		@@ -0,0 +1,7 @@
		function hasLabelProperty(v: unknown): v is { label: unknown } {
		return typeof v === 'object' && v !== null && 'label' in v;

Refactor front end Meta class and a lot of surrounding code. #1109

Refactor front end Meta class and a lot of surrounding code. #1109

Conversation

seancolsen commented Mar 1, 2022 • edited Loading

Context

Goals

Caveats

Checklist

Developer Certificate of Origin

seancolsen Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

pavish Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

pavish Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

I tracked down the "when" of the problem:

Regarding the "why" of the problem:

The hack:

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

pavish Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 2, 2022 • edited Loading

Codecov Report

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

pavish Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavish left a comment • edited Loading

Choose a reason for hiding this comment

Refactor front end `Meta` class and a lot of surrounding code. #1109

Refactor front end `Meta` class and a lot of surrounding code. #1109

seancolsen commented Mar 1, 2022 •

edited

Loading

seancolsen Mar 1, 2022 •

edited

Loading

pavish Mar 2, 2022 •

edited

Loading

pavish Mar 2, 2022 •

edited

Loading

pavish Mar 3, 2022 •

edited

Loading

codecov-commenter commented Mar 2, 2022 •

edited

Loading

pavish Mar 3, 2022 •

edited

Loading

pavish left a comment •

edited

Loading