-
Notifications
You must be signed in to change notification settings - Fork 3
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
TS Safe Assignment, Sanity CMS Basic Library & Splitting Utils Package #57
base: dev
Are you sure you want to change the base?
Conversation
…ypto and @session/util-logger
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.
Great work man! Couple of comments
* return result; | ||
* ``` | ||
*/ | ||
export async function safeTry<T, E = Error>(promise: Promise<T>): Promise<[null, T] | [E, null]> { |
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.
Nice code although it is interested this needed to become a tool but I guess you thought you would use it a lot more in future.
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 decided to give it a go, would be incredible for the general state management and integrity problem with decentralized apps, plus I think it has a lot of potential for general js stability improvements. It's a basic implementation of the sage 0 safe assignment operator
TypeScript proposal:
packages/util-js/tests/try.spec.ts
Outdated
it('should return a tuple with an error if the promise rejects', async () => { | ||
expect.assertions(5); | ||
|
||
const result = await safeTry(Promise.reject(new Error('foo'))); |
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.
This is nitpicking but you could just create the error once right? const fooError = new Error('foo');
packages/util-js/env.ts
Outdated
*/ | ||
export const setEnvironment = (environment: Environment) => { | ||
if (!isEnvironment(environment)) { | ||
console.warn(`Invalid environment: ${environment}`); |
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.
Not console.error?
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.
Just a warning, have updated to say it defaults to dev
now
|
||
/** | ||
* Retrieves the current environment based on the value of {@link env}. If the environment is not | ||
* set, it defaults to fetches it from the NEXT_PUBLIC_ENV_FLAG environment variable. |
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.
Are we planning to use util-js in session-desktop? If so might be best to not have the NEXT flag hardcoded. Maybe you can pass it through?
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.
Yeah will have to work out a better solution, the only env variables client-side next can see are ones prefixed with NEXT_PUBLIC_
. When we use this on desktop we can decide on what to use cross-environment.
|
||
This package is a utility library for common functions. | ||
This package is a js utility library for common functions. |
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.
Is this not technically ts? 😂
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 mean it's written in ts but all the functionality is transpiled to js so it is a js utility library. imo something is only a ts library if its explicitly for ts, not js. eg: sanity-types vs sent-staking-js
packages/sanity-cms/lib/preview.ts
Outdated
import type { RouteSchemaType } from '../schemas/fields/groups/route'; | ||
|
||
// Customise this function to show the correct URL based on the current document | ||
function getPreviewUrl(route: RouteSchemaType) { |
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.
This is not used. Did you mean to export 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.
Also should we check if window exists first?
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.
deleted :)
* relying on global type declarations and this caused a global type conflicts, forcing the `next` | ||
* and `cache` properties to be ONLY `undefined` as TS couldn't find the `next property in all | ||
* @see {RequestInit} declarations (it's only in the next.js declaration, not node, web lib, or | ||
* workers versions). |
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.
You mad?😂
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.
hahaha i may have been a little fed up at that point. but im not wrong. :)
packages/sanity-cms/lib/config.ts
Outdated
projectId: string; | ||
dataset: string; | ||
/** Must match the route of your Studio */ | ||
studioBasePath: string; |
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.
studioBasePath: string; | |
/** Must match the route of your Sanity Studio instance */ |
@@ -0,0 +1,13 @@ | |||
import { type ReactNode, Suspense } from 'react'; | |||
|
|||
export const dynamic = 'force-static'; |
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.
What does this do? Can you docstring it please?🙏
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.
added
|
||
// If the `_type` is `post`, then all `client.fetch` calls with | ||
// `{next: {tags: ['post']}}` will be revalidated | ||
revalidateTag(body._type); |
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.
Yeah once I'm back you are going to have to teach me this stuff. Impressive but also a bit beyond my current knowledge
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.
haha sounds good, theres a lot of next magic going on
Summary
Split the utility package into tighter scoped packages, created a generic Sanity CMS library, and implemented basic version of the TypeScript safe assignment operator proposal to investigate its usefulness and hopefully enhance state management stability for decentralized apps.
Key Updates
Utility Package Refactoring:
@session/util
package into modular packages for JavaScript utilities, crypto functions, and logging to improve separation of concerns.Sanity CMS Integration:
API Enhancements:
Error Handling & Safe Assignment Operator Proposal:
safeTry
function for safer error handling and reduced risk of unhandled exceptions.Miscellaneous Improvements: