Skip to content
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

Add Project.add helper #400

Merged
merged 9 commits into from
Nov 14, 2023
Merged

Add Project.add helper #400

merged 9 commits into from
Nov 14, 2023

Conversation

alexdavid
Copy link
Contributor

No description provided.

@soenkehahn
Copy link
Contributor

Trying this out in both #393 and #397 it seems that this solves our use-cases there. I think there's two open questions:

  • Is .add the best name? Some alternatives are .with, .modify, .withSelf. Any other ideas? I kind of like both .add and .with. Although, technically, this could be used to not add something but somehow modify the project in another way, including removing things. So maybe .add is not the best name.
  • Is there a good way to provide a type alias for these plugins? Something like type Plugin<T extends ProjectData, U extends ProjectData> = (project: T) => U. It'd be nice to be able to say e.g. vite.addBuilder has type Plugin<{ node_modules: Package }, { viteBundle: Package }>. (Modulo names. I don't like the name Plugin, any better ideas?) But toying around with that now I think that providing a type synonym that works well is not that easy. So maybe this PR should include a type synonym and have some type tests for that?

@soenkehahn soenkehahn force-pushed the ad/plugin-system branch 2 times, most recently from ebbecc6 to 26003ca Compare November 13, 2023 14:41
@soenkehahn soenkehahn linked an issue Nov 13, 2023 that may be closed by this pull request
ts/project.test.ts Outdated Show resolved Hide resolved
@jkarni
Copy link
Contributor

jkarni commented Nov 13, 2023

Is .add the best name? Some alternatives are .with, .modify, .withSelf. Any other ideas? I kind of like both .add and .with. Although, technically, this could be used to not add something but somehow modify the project in another way, including removing things. So maybe .add is not the best name.

How about apply? It seems the most accurate.

@soenkehahn soenkehahn marked this pull request as ready for review November 14, 2023 00:02
@soenkehahn
Copy link
Contributor

How about apply? It seems the most accurate.

We now changed the api of .add (and Plugin) so that it actually doesn't allow removing fields from a Project, see

garn/ts/project.ts

Lines 222 to 230 in a515a1e

add<T extends ProjectData, Additions>(
this: T,
fn: Plugin<Additions, T>,
): Omit<T, keyof Additions> & Additions {
return {
...this,
...fn(this),
};
},
. (Maybe it's possible by doing something weird, like setting a field to undefined, but it's certainly not an obvious way to use the API.) So I think the name .add makes a lot of sense and is consistent with the other functions that start with add.

Copy link
Contributor Author

@alexdavid alexdavid left a comment

Choose a reason for hiding this comment

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

This looks good to me too. Going to merge

@alexdavid alexdavid merged commit f075a8a into main Nov 14, 2023
28 checks passed
@alexdavid alexdavid deleted the ad/plugin-system branch November 14, 2023 04:27
@soenkehahn soenkehahn mentioned this pull request Nov 15, 2023
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.

Provide an easy way to bundle up changes to projects
3 participants