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

[WIP] Code Generation Plugin System #572

Merged
merged 12 commits into from Mar 1, 2024

Conversation

fimion
Copy link
Contributor

@fimion fimion commented Feb 11, 2024

This is the first draft of a plugin system for the code generation part of Oazapfts (Discussion can be found in #562)

Copy link
Collaborator

@Xiphe Xiphe left a comment

Choose a reason for hiding this comment

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

This is looking great already!

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 13, 2024

@fimion heads up I'm currently splitting the package: #576
In order to prevent merge conflicts it might be wise to not work on big refactors until that one is merged. Let me know when you need something 👍

@fimion
Copy link
Contributor Author

fimion commented Feb 13, 2024

Cool, I'll hold off a bit, let me know if you need/want help

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 13, 2024

@fimion

The PR is ready to merge from my end, it just needs another pair of eyes and I think it would make sense when a few people could beta-test oazapfts@6.0.0-beta.1 and @oazapfts/runtime@1.0.0-beta.1.

So when you like, a review and/or a test would definitely help getting this out 🤙

@fimion
Copy link
Contributor Author

fimion commented Feb 13, 2024

I'll take a look this morning!

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 14, 2024

Thanks for incorporating my feedback!

Mentally iterating on this...

With the idea in the back of my head that I would like to put all oazapfts logic into a default plugin I wonder:

Should we try to create an architecture that...

  1. allows plugins to set their priority on a given hook?
  2. allow plugins to cancel all other plugins on a given hook?

The Idea came from looking at the afterWriteToFile... it's a constructed scenario but what if there would be a plugin that writes the output directly to #cloudStorage. Ideally that plugin would want to run before the default implementation and then also declare that the original logic of writing to disk should be skipped.

So to support this, I'd suggest to use some sort of a priority setting on a hook rather then before... and after... also there would need to be a way to bail.

maybe we can leverage tapable or a similar library? (I don't have experience with tapable, just was the first solid looking solution that I found)

An API for this approach will likely look more imperative, for example:

import { createPlugin } from "oazapfts";

export default createPlugin(({ apiGenerator }) => {
  apiGenerator.hooks.convertSchema.tap("just-use-any", () => {
    return ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword);
  });

  apiGenerator.hooks.write.tapPromise(
    "write-to-cloud",
    { stage: 0 },
    async ({ fileContents, fileName }) => {
      await myCloud.write(fileContents, fileName);
      return true;
    },
  );
});

Not feeling 100% confident with this. Might also be overkill 🤷 what do you think @fimion ?

@fimion
Copy link
Contributor Author

fimion commented Feb 17, 2024

Thanks for incorporating my feedback!

Mentally iterating on this...

With the idea in the back of my head that I would like to put all oazapfts logic into a default plugin I wonder:

Should we try to create an architecture that...

  1. allows plugins to set their priority on a given hook?
  2. allow plugins to cancel all other plugins on a given hook?

I think that priority is a great idea (I was planning on the priority being the order they are loaded, but different steps may need different priority)

If we are wanting to move the code generation process to be a default plugin, then we need to have a way to block cancelling certain steps. or we need to have a baseline set of functionality that always happens before breaking out the default functionality into a plugin itself. Or maybe not, I don't know. I think I'm having trouble thinking of a case where I would want to skip the OAS->AST generating steps, but that is probably just me having a limited perspective.

The Idea came from looking at the afterWriteToFile... it's a constructed scenario but what if there would be a plugin that writes the output directly to #cloudStorage. Ideally that plugin would want to run before the default implementation and then also declare that the original logic of writing to disk should be skipped.

at work we've currently got a custom script that breaks out code generation into separate files, so being able to use something like this would be great.

So to support this, I'd suggest to use some sort of a priority setting on a hook rather then before... and after... also there would need to be a way to bail.

maybe we can leverage tapable or a similar library? (I don't have experience with tapable, just was the first solid looking solution that I found)

I'll take a look.

An API for this approach will likely look more imperative, for example:

import { createPlugin } from "oazapfts";

export default createPlugin(({ apiGenerator }) => {
  apiGenerator.hooks.convertSchema.tap("just-use-any", () => {
    return ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword);
  });

  apiGenerator.hooks.write.tapPromise(
    "write-to-cloud",
    { stage: 0 },
    async ({ fileContents, fileName }) => {
      await myCloud.write(fileContents, fileName);
      return true;
    },
  );
});

Not feeling 100% confident with this. Might also be overkill 🤷 what do you think @fimion ?

I think if we can get an interface like this typed correctly, it would probably be the best solution long term.

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 19, 2024

Cool! Thank you for the feedback!

If we are wanting to move the code generation process to be a default plugin, then we need to have a way to block cancelling certain steps. or we need to have a baseline set of functionality that always happens before breaking out the default functionality into a plugin itself. Or maybe not, I don't know. I think I'm having trouble thinking of a case where I would want to skip the OAS->AST generating steps, but that is probably just me having a limited perspective.

I also don't have a concrete scenario for this. My motivation for moving the core into a plugin is

  1. I hope that it will make the core logic easier to maintain and to reason about
  2. I hope this will enable use cases that we don't know about yet, while doing so 👆. Meaning a win-win scenario.

@fimion
Copy link
Contributor Author

fimion commented Feb 19, 2024

@Xiphe I'm also looking at https://github.com/unjs/hookable as an option. It's typescript and is used by nuxt (I'm partial to vue, so i'm extremely biased here) but i think both of these could be good options, and the way they would get used would be very similar.

Would you prefer tapable or hookable?

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 19, 2024

Skimming over it, hookable seems to have no support for manual ordering as tapable has with stage. Which would be better suited for a beforeHook => nativeImplementation => afterHook architecture.

Where tapable seems to order taps by stage which would allow us to tap the native implementation to stage: 10 and then plugins might have finer control on wether they want to be the VERRY FIRST or rather somewhere in the middle.
(Again: I have not used this, so I might be completely off in both: how this is supposed to work as well as how useful it is)

Native typescript is a plus. Also I'm actually not a fan of the tapable ergonomics, it just seemed to cover the feature base we might need here and looking at the repo activity of tapable its either done or abandoned 🤷

I'd say let's not block ourselves with this decision, it seems to me as both point towards a similar direction which I like. I'd assume switching should not be a huge deal.


That's all to say: I'm happy with both :)

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 19, 2024

Might also be straight forward enough to not use a lib here and just maintain a collection of callbacks ourselves 🤔

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 23, 2024

Hi @fimion

I gave this another thought...

We might also just create the plugin interface as a collection of arrays with callbacks in it.

type InitFn = (args: unknown) => unknown;
type ConvertSchemaFn = (args: unknown) => unknown;
type Oazapfts = {
  init: InitFn[];
  convertSchema: ConvertSchemaFn[];
};

A plugin could then just manipulate these arrays:

definePlugin((o: Oazapfts) => {
  // add to the end 
  o.init.push(myImplementation);
  // replace all existing
  o.convertSchema = [letMeHandleAllOfThis!];
};

I think this is the simplest solution that still provides a ton of flexibility.

@fimion
Copy link
Contributor Author

fimion commented Feb 28, 2024

Okay @Xiphe I've made my first pass using the tapable package with proper types.

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 29, 2024

Awesome! This looks like a great base to build upon! I'm not too sure if we actually need the before as we can set the stage when tapping in from a plugin but we can keep it for now.

How would you suggest we continue from here?

@fimion fimion marked this pull request as ready for review February 29, 2024 13:34
@fimion
Copy link
Contributor Author

fimion commented Feb 29, 2024

I think either this should be merged in for #584 or we should finish #584 then begin breaking out the default behavior into a plugin here. I'll also need to make a plugin loading system, which will change how the initial cli loading works. We can discuss that further if you'd like.

@Xiphe
Copy link
Collaborator

Xiphe commented Feb 29, 2024

Yeah I like the idea of merging with #584!

@Xiphe
Copy link
Collaborator

Xiphe commented Mar 1, 2024

Gonna merge this to main because:

  • Everything is in the __future__ folder
  • This way GitHub understands @fimion as a contributor and CI will run automatically in future runs

Going forward I suggest we:

  • iterate on the base plugin system from main (PRs to main)
  • iterate on modularizing the source and make the plugin system work on alpha
    • removal of the ApiGenerator class is a somewhat breaking change as people had been augmenting that one for custom behavior.
    • thats why I would like to release the finished plugin support along with removing the ApiGenerator

@Xiphe Xiphe merged commit 3535b1b into oazapfts:main Mar 1, 2024
1 check passed
Xiphe added a commit that referenced this pull request Mar 10, 2024
in order to completely remove and disincentivize use of class state
in favor of OazapftsContext

as a preparation for plugin architecture

BREAKING CHANGE:
ApiGenerator Class has been removed

ref #572
Copy link

🎉 This PR is included in version oazapfts-v6.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants