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

Plugins Must Declare Annotations [WIP] #1386

Draft
wants to merge 2 commits into
base: release-1.0.0
Choose a base branch
from

Conversation

markwpearce
Copy link
Collaborator

@markwpearce markwpearce commented Jan 1, 2025

Addresses #1348

Plugins must declare their annotations, so that can be validated.

  • Add Documentation about Annotations
  • Fix Deprecated docs with references to old plugin events
  • Agree on format of annotation declarations
  • Etc

@markwpearce markwpearce added this to the v1.0.0 milestone Jan 1, 2025
@markwpearce
Copy link
Collaborator Author

@TwitchBronBron

The docs I added follow along with the description from the issue, but I'm thinking it may be easier to deal with annotations that are provided as Types?

eg:

const myAnnotation = new TypedFunctionType()
myAnnotation.name = 'MyAnnotation';
myAnnotation.params = [
    {name: 'id', type: StringType.instance},
    {name: 'extraData', type: new AssociativeArrayType(), optional: true}
];

this.annotations = [myAnnotation]

(perhaps we could update the constructor or create a helper function to create it all at once?)

This would mean we wouldn't have to parse the declaration, but instead use it directly... and wouldn't need to have extra error handling on bad parsed values, etc.

Copy link
Collaborator

@luis-soares-sky luis-soares-sky left a comment

Choose a reason for hiding this comment

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

Mostly off-topic feedback 👼

Comment on lines 77 to +85
- `beforePrepublish`
- `afterPrepublish`
- `beforePublish`
- `beforeProgramTranspile`
- `beforeSerializeProgram`
- `beforeBuildProgram`
- For each file:
- `beforeFileTranspile`
- `afterFileTranspile`
- `afterProgramTranspile`
- `afterPublish`
- `beforePrepareFile`
- `afterPrepareFile`
- `afterBuildProgram`
- `afterSerializeProgram`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The remaining API methods are all named with a specific order of "time" > "subject" > "action", i.e. beforeProgramCreate, afterFileValidate, etc. But these either disregard the "subject" entirely, or have their "subject" and "action" reversed.

Also, not sure I understand the difference between the program's serialize and build steps, and the need for the beforePrepublish and afterPrepublish steps. Is there a need for all of these?

- `afterProgramCreate`
    - (...)
    - `afterProgramValidate`
- `beforeProgramPublish`
    - For each file:
        - `beforeFilePrepare`
        - `afterFilePrepare`
- `afterProgramPublish`
- `beforeProgramDispose`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree the naming should be more consistent.

@TwitchBronBron can we make this change?
also, maybe you could add a little more to these docs to explain what these events represent?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm definitely open to tweaking some of these names so they're more consistent.

With regard to the prepublish, serialize, build steps, they each have their own purpose, but we should definitely add more documentation around them to better explain their differences.

Comment on lines +101 to +102
- `beforeProvideFile`
- `afterProvideFile`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above:

- `beforeFileProvide`
- `afterFileProvide`

Copy link
Member

Choose a reason for hiding this comment

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

Here, file isn't the subject—we're asking plugins to provide a file, not the files to provide something. So the term subject is a bit different, though I can see it being read slightly that way.

But perhaps it doesn't matter, and we should just align on the naming convention anyway?

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.

3 participants