-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore(POC): simpler / more flexible api #181
base: main
Are you sure you want to change the base?
Conversation
|
commit: |
@@ -10,28 +10,12 @@ import { | |||
object, | |||
variables, | |||
exports, | |||
kit | |||
kit as kitJs |
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 bad, but works for the moment. This is required since our workspace
has a kit
property and this import is also named kit
packages/adders/paraglide/index.ts
Outdated
name: () => 'project.inlang/settings.json', | ||
condition: ({ cwd }) => !fs.existsSync(path.join(cwd, 'project.inlang/settings.json')), | ||
content: ({ options, content }) => { | ||
run: ({ api, cwd, options, typescript, kit, dependencyVersion }) => { |
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.
run: ({ api, cwd, options, typescript, kit, dependencyVersion }) => { | |
run: ({ files, packages, scripts, cwd, options, typescript, kit, dependencyVersion }) => { |
Open for discussion: Only one export, or multiple? I had implemented both locally, but decided to go with api
as files
and scripts
only had one property. It might be more future proof, if we export all three things separately.
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 would probably name them as just sv
and file
. sv
would encapsulate packages
and scripts
and file
would just be for file handling.
but maybe having a unified sv
that does it all isn't so bad... hmmm, need to think on 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.
interesting approach to just name it sv
, i like that.
Although i wouldn't be sure about combining packages
and scripts
and leaving files
separately. I would suggest that we do all or nothing here.
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 issue with not letting it have it's own namespace is that each method now gets a bit more verbose
ex:
sv.updateFile
sv.createOrUpdateFile
sv.createFile
as opposed to:
file.update
file.createOrUpdate
file.create
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.
But wouldn't we have exactly the same problem with script
and package
?
Additionally, package
is a reserved keyword in strict mode, which would make it necessary to use the plural or something completely different like dependencies
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.
Another option would be to allow sv.file.update()
to group it even more. But in that case i would probably start destructuring the object, basically leaving us with an option we already have.
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.
pkg
or pkgJson
or something would be another option
I haven't made up my mind about splitting vs combining, but do feel we should be consistent. maybe we should ask Rich for feedback after svelte.dev is updated as he's quite good with API design
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.
thinking on it more, I think a single api via sv
should be fine:
sv.file // creates or updates files - see other comment down below
sv.dependency // adds dep
sv.devDependency // adds devDep
sv.execute // runs any scripts
sv.dependsOn // runs another adder(?)
this really does look and feel much nicer!! there are still things i would love to improve after this, such as using an ast manipulating api like this: const { script, css, generateCode } = parseSvelte(content);
script.importNamed('../adder-template-demo.txt?raw', { demo: 'Demo' });
css.addClass('foobar', { 'background-color': 'blue' });
return generateCode({ script: script.generateCode(), css: css.generateCode() }); after that, i think we'll be super close to having a library that's ready for community integrations |
Regarding that bit of code that you removed back to the top: I moved it on purpose because it's only used in that one place and nowhere else, so i thought it was unnecessary polluting global stuff. But whatever works for you, i don't have a strong opinion on that.
I agree, there is lot's of improvement potential in the ast apis as well. Overall i like where this would be going. But as you said, one step at a time. |
yea, i much prefer keeping actual constants in the top scope, especially in this case so it's not polluting the sections with logic |
so one issue with this new api is that we'll no longer be able to determine if an integration has been previously installed as the packages are no longer static values that we can easily examine. see: cli/packages/cli/commands/add/index.ts Lines 309 to 327 in 4812956
we're accessing the packages field and checking the package.json to see if they're installed.
the not exactly sure how this can be worked around yet, but this is something we definitely needs to solve |
I wasn't aware anymore that we had this in place. I was initially going to suggest something like this run: ({ sv, cwd, options, typescript, kit, dependencyVersion }) => {
if(!!dependencyVersion('drizzle-orm'))
sv.dependsOn('drizzle)
// do other file updates
} That would increase flexibility as well. But this won't work because we will only be able to determine the integrations it depends on while executing the adder. But in general I do think a functional approach like this would be best, maybe by converting our existing |
I just pushed a rough implementation of the new api (without "dependsOn" detection). This obviously requires more cleanup, but i wanted to make sure we don't miss anything. Running the paraglide adder in it's current state seems to create the same results as our current implementation. Only had a rough look though, should probably diff the changes later for to make sure this is the case. |
const ext = typescript ? 'ts' : 'js'; | ||
if (!kit) throw new Error('SvelteKit is required'); | ||
|
||
sv.dependency('@inlang/paraglide-sveltekit', '^0.11.1'); |
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.
perhaps it should be called addDependency
?
if (!fs.existsSync(path.join(cwd, 'project.inlang/settings.json'))) { | ||
sv.updateFile('project.inlang/settings.json', (content) => { |
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 think we could combine these into a single API that adds the file if it doesn't exist. (maybe createFile
, updateFile
, and createOrUpdateFile
? or updateFile
could take an option describing whether to throw an error or overwrite if an existing file is found?)
if (!fs.existsSync(path.join(cwd, 'project.inlang/settings.json'))) { | |
sv.updateFile('project.inlang/settings.json', (content) => { | |
if (!fs.existsSync(path.join(cwd, 'project.inlang/settings.json'))) { | |
sv.updateFile('project.inlang/settings.json', (content) => { |
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 think a singular API makes sense (similar to how the name: () => '...'
field operates today where it'll only create the file if the returned string is not empty). it could also have a second parameter that indicates the kind:
sv.file('project.inlang/settings.json', (content: string, kind: 'create' | 'update') => {
if (kind === 'create') {
// ...
}
if (kind === 'update') {
// ...
}
});
most of the time though, you wouldn't care about whether the file is created or updated, so it can simply be this:
sv.file('project.inlang/settings.json', (content: string) => {
// ...
});
Closes #85
This is a POC to start visualizing stuff and initiate discussions. Besides of some new types & adapting the
paraglide
adder, I haven't done any implementation. So expect plenty of type errors and failing pipelines.Up until now I was keeping my mouth shut about #85 as I had many doubts if this was the right way going forward. Also I had a hard time imagining how this would look in a real adder. Now that this is done with this POC it was eye opening how many flexibilities this will bring us & the community. Besides copying some code, there are nearly zero changes that affect the functionality of the adders. Since most of the functionality already exists it should be straight forward to implement this, once we come up with the "final" api. Most of the work probably needs to be done inside the adders, as there is lots of stuff that needs to be moved.
Edit: The diff looks way worse than it actually is. Consider opening the whole file or viewing in VScode