Skip to content

contextPath relative to CWD and resolvers return readonly array #231

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

Merged
merged 4 commits into from
Oct 27, 2022

Conversation

vejrj
Copy link
Contributor

@vejrj vejrj commented Oct 26, 2022

No description provided.

@vladar
Copy link
Contributor

vladar commented Oct 26, 2022

I think we should consider having Readonly in PromiseOrValue type instead. This will make generated types a bit more readable.

Example in TS playground

@freiksenet @vejrj what do you think?

@freiksenet
Copy link
Contributor

Yeah, this looks good. Let's do that.

if (!contextImport) {
return;
}
const contextDir = path.join(path.dirname(inputPath), contextImport);
const contextDir = path.join(process.cwd(), contextImport);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI for future changes, this is equivalent to path.resolve(contextImport)

factory.createTypeReferenceNode(
factory.createIdentifier("ReadonlyArray"),
[type as ts.TypeNode],
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc, the way to do this with modern TS and through the API is something like:

factory.createTypeOperatorNode(ts.SyntaxKind.ReadonlyKeyword, factory.createArrayTypeNode(type))

Which would produce:

readonly X[]

This reverts commit 0a2968d.
export type ImmutableSet<T> = ReadonlySet<Immutable<T>>;
export type ImmutableObject<T> = { readonly [K in keyof T]: Immutable<T[K]> };

export type PromiseOrValue<T> = Immutable<T> | Promise<Immutable<T>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to wonder how expensive this might be vs the previous inlined solution, when compiling or trying to get instant feedback from the LSP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are other problems with it as well (posted in a separate comment). Let's stick with a simple approach then (at least for now)

@vladar
Copy link
Contributor

vladar commented Oct 27, 2022

Generic immutable seems to be a hard thing to do right (see full discussion in this thread).

So, for now we'll have to stick with explicit ReadonlyArray. We can revisit it a bit later for our specific case, that doesn't really require a generic solution.

@vejrj vejrj merged commit f1936be into main Oct 27, 2022
@vejrj vejrj deleted the supermassive-codegen-resolvers-return-type-fix branch October 27, 2022 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants