@location tag #3011
@location tag #3011
Conversation
* Forwardref-fix: Adds forwardRef to ViewerWithMenuBar and ViewerBody
🦋 Changeset detectedLatest commit: e1ee728 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| toString(): string { | ||
| return Object.entries(this.value) | ||
| .map(([key, value]) => `${key}: ${value?.toString()}`) | ||
| .join(", "); |
There was a problem hiding this comment.
Could also write this out. The previous setup would also work, though it's nice to organize it in ValueTagsWrapper, which can do a better job. (It can convert to Values, for use with the Library)
|
After this goes in, we should change |
There was a problem hiding this comment.
So this made me realize that ValueTagsWrapper (or tagUtils I proposed, whatever) could allow me to avoid circular dependency issues in #2978.
I'm also not sure how much sense ValueTags class makes now; we could convert everything to the functional style and pass ValueTagsType-shaped object everywhere, instead of having to organize things based on whether we have circular dependencies or not.
But it's not very important for now, feel free to merge.
| } | ||
|
|
||
| // This class is meant for functions that depend on Value. BaseValue depends on ValueTags, so we can't call many functions there. Instead, we call them here. | ||
| export class ValueTagsWrapper { |
There was a problem hiding this comment.
I think you could safely skip OOP here and use a module-as-plain-object:
export const tagUtils = {
toVDict: (tags: ValueTags) => { ... },
toValueMap: (tags: ValueTags) => { ... },
toLocation: (tags: ValueTags) => { ... },
}And then return tagUtils.toValueMap(tags).
(AFAICT the only function you need here is toValueMap, but exporting many small functions one by one can be annoying and less readable, so a top-level object is more convenient)
Later we might have to change it back to being a |
One key consideration is if we can auto-add @location on decorators. If we can, then I think this should be named
@location, as I don't expect it to be frequently used. If we can't, then I think we should name it@loc.There's a significant issue with this PR, as I had to make a new
ValueTagsWrapperclass to handle some functions onValueTags, due to an issue of circular dependencies. Curious if you have preffered ways of doing this. I also considered havingBaseValuerely on an abstract class version ofValueTags, but that seemed like too much.Tests are failing because the new
toStringformat is different. If this PR is approved, I'll fix these.