-
-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor front end Meta
class and a lot of surrounding code.
#1109
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
export default class ImmutableMap< | ||
Key extends string | number | boolean | null, | ||
Value, | ||
> { | ||
private map: Map<Key, Value>; | ||
|
||
constructor(i: Iterable<[Key, Value]> = []) { | ||
this.map = new Map(i); | ||
} | ||
|
||
/** | ||
* This method exists to allow us to subclass this class and call the | ||
* constructor of the subclass from within this base class. | ||
* | ||
* If there's a way we can use generics to avoid `any` here, we'd love to | ||
* know. | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
private getNewInstance(...args: any[]): this { | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-explicit-any | ||
return new (this.constructor as any)(...args) as this; | ||
} | ||
|
||
/** | ||
* The value supplied here will overwrite any value that is already associated | ||
* with `key`. | ||
*/ | ||
with(key: Key, value: Value): this { | ||
const map = new Map(this.map); | ||
map.set(key, value); | ||
return this.getNewInstance(map); | ||
} | ||
|
||
/** | ||
* When the same keys exist in within the entries of this instance and the | ||
* entries supplied, the values from the entries supplied will be used instead | ||
* of the values in this instance. This behavior is consistent with the `with` | ||
* method. | ||
*/ | ||
withEntries(entries: Iterable<[Key, Value]>): this { | ||
const map = new Map(this.map); | ||
[...entries].forEach(([key, value]) => { | ||
map.set(key, value); | ||
}); | ||
return this.getNewInstance(map); | ||
} | ||
|
||
/** | ||
* If `key` already exists, its corresponding value will remain. If `key` does | ||
* not exist, then the value supplied here will be used. | ||
*/ | ||
coalesce(key: Key, value: Value): this { | ||
return this.has(key) ? this : this.with(key, value); | ||
} | ||
|
||
/** | ||
* When the same keys exist in within the entries of this instance and the | ||
* entries supplied, the values from this instance will be used instead of the | ||
* values from the supplied entries. This behavior is consistent with the | ||
* `coalesce` method. | ||
*/ | ||
coalesceEntries(other: Iterable<[Key, Value]>): this { | ||
const map = new Map(this.map); | ||
[...other].forEach(([key, value]) => { | ||
if (!this.has(key)) { | ||
map.set(key, value); | ||
} | ||
}); | ||
return this.getNewInstance(map); | ||
} | ||
|
||
without(key: Key): this { | ||
const map = new Map(this.map); | ||
map.delete(key); | ||
return this.getNewInstance(map); | ||
} | ||
|
||
has(key: Key): boolean { | ||
return this.map.has(key); | ||
} | ||
|
||
get(key: Key): Value | undefined { | ||
return this.map.get(key); | ||
} | ||
|
||
get size(): number { | ||
return this.map.size; | ||
} | ||
|
||
keys(): IterableIterator<Key> { | ||
return this.map.keys(); | ||
} | ||
|
||
values(): IterableIterator<Value> { | ||
return this.map.values(); | ||
} | ||
|
||
entries(): IterableIterator<[Key, Value]> { | ||
return this.map.entries(); | ||
} | ||
|
||
[Symbol.iterator](): IterableIterator<[Key, Value]> { | ||
return this.entries(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
function hasLabelProperty(v: unknown): v is { label: unknown } { | ||
return typeof v === 'object' && v !== null && 'label' in v; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
export function hasStringLabelProperty(v: unknown): v is { label: string } { | ||
return hasLabelProperty(v) && typeof v.label === 'string'; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,12 @@ | |
undefined; | ||
|
||
// Total number of pages. | ||
// | ||
// 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unconventional for other frameworks but the whole point of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I disagree. I would say the point of This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point regarding For the Even though We can remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That would be problematic in the same way. If I were designing this from scratch, I'd make a 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 In our code currently, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not necessarily.
We'd pass down
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:
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 We can solve this simply by exporting some util methods, as I mentioned above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// > calculation in the parent and pass it down to this component. | ||
export let pageCount = 0; | ||
|
||
// ARIA Label for component | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seancolsen
I tracked down the "when" of the problem:
All the callbacks of subscribers of
tabs
andactiveTab
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 bindingtabs
andactiveTab
toTabContainer
, or remove the#if
condition fortabs.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
andactiveTab
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During our sync call, I'm also curious to explore this point above more, because that definitely does not match not my understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I added the call to
beginUpdatingUrlWhenSchemaChanges
, I was observing that sometimesBase.svelte
would re-render itsTabContainer
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 inmaster
. 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has to do with
TabContainer
being created and destroyed, since it is wrapped within an#if
block. The upstream bug calls all subscribers oftabs
andactiveTab
stores, each timeTabContainer
is created.Assume you opened a table in
Schema 1
, which creates theTabContainer
andTabList
instance, which inturn updates the url. Then you openSchema 2
, which contains no tables, soTabContainer
gets destroyed. Note, we create a newTabList
instance when a schema is opened for the first time, so the url update will happen whenSchema 2
is opened.When we switch back to
Schema 1
, theTabContainer
gets created again, which updates the url. If you then open a table inSchema 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!