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

DON'T MERGE: User plugins proposal and prototype #497

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

blake-mealey
Copy link
Contributor

This is a proposal and prototype implementation for a solution to #55.

The goal with this PR is primarily to receive feedback on the proposed design document. If we can come to a consensus on the design, I would be happy to start submitting PRs to start the implementation of it.

Please see the included PLUGIN-DESIGN.md file for the proposal, and the code changes for the prototype. To try the prototype locally, you can build/serve/whatever the project test-projects/plugins project (e.g. cargo run build test-projects/plugins -o test.rbxlx).

The implementation is very prototypesque and not the cleanest code, and does not fully implement the proposed design. My goal with it was primarily to validate that the proposal is reasonable to achieve given the internal implementation of Rojo which I didn't understand previously. I consider this to be validated.

The prototype implements the following parts of the proposal:

  • The new top-level plugins field of the project file (both raw strings and objects with source and options fields), but not URL sources
  • Plugin environment and loading mechanism (only local plugin files can be loaded)
  • Minimal plugin instance hooks (only supports the middleware and load hooks)
  • Minimal plugin library (only inlcudes the readFileAsUtf8 function)

Thanks!

@blake-mealey blake-mealey marked this pull request as draft October 28, 2021 23:38
Comment on lines +99 to +108
- `resolve(id: string) -> string`
- **Optional**: Takes a file path and returns a new file path that the file should be loaded
from instead. The first plugin to return a non-nil value per id wins.
- `middleware(id: string) -> string`
- **Optional**: Takes a file path and returns a snapshot middleware enum to determine how Rojo
should build the instance tree for the file. The first plugin to return a non-nil value per
id wins.
- `load(id: string) -> string`
- **Optional**: Takes a file path and returns the file contents that should be interpreted by
Rojo. The first plugin to return a non-nil value per id wins.
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a quick read over this doc, making plugins race against each other seems a bit odd, and could lead to some nondeterminism in different runs (plugin 1 was quick the first time round, but something happened and it was slow the second time, so its output is now lost).

Maybe it would be nicer to have a specific ordering of the plugins? the plugins field in the project file format even leads to this - a user can order the plugins in the array depending on how they want them to run. (e.g. we have plugin 1 which firstly transforms some code, then we let plugin 2 run afterwards which takes the output from plugin 1, and minifies it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure where you got the idea that the plugins would race each other but your suggestion is actually how it's already intended to work. The plugins array allows the user to determine the plugin order and that order is used each time to deterministically determine the results.

Copy link
Contributor

Choose a reason for hiding this comment

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

The proposal said "the first plugin to return a non nil value wins" but maybe I misunderstood this - I thought it meant all plugins run at the same time and the first one to return a value will be used. I now realise I think it means that once a value is returned, it no longer calls for values from other plugins? My apologies!

Still, I think the second point mentioned may still make sense. If plugin 1 transforms require paths, and plugin 2 minifies the whole code, then we want to run both plugins (plugin 1 then 2), rather than just stopping after 1. I haven't looked into implementation details though so if this actually is what is happening already then my bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I understand the confusion now. Yes, that sentence means "run them in order and stop once one has returned a value."

Yes, that is a good point! I will need to get my head back into how this proposal works (it's been a while) and come back with a response on it but that's definitely a use case that should be supported.

@LPGhatguy LPGhatguy added status: on hold This is something we're not going to do right now, but might do later. labels Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: on hold This is something we're not going to do right now, but might do later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants