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 hooks #221

Merged
merged 9 commits into from
Aug 20, 2021
Merged

Add hooks #221

merged 9 commits into from
Aug 20, 2021

Conversation

techninja1008
Copy link
Contributor

@techninja1008 techninja1008 commented Aug 6, 2021

Hi all. Put this together as I felt trunk lacks a good way to extend the build process with extra steps. I'm aware #104 includes a longer-term vision for a plugins system, but simple command-line hooks should provide for many use cases (including mine) for now.

Currently, this PR only includes the feature as-is and an extra example (yew-tailwindcss) that showcases it in order to produce a custom tailwindcss build that is pruned to only include classes used by the example. I'd like a bit of feedback on the design before completing the checklist below.

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).
  • Updated site content with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 🤓.

@thedodd
Copy link
Member

thedodd commented Aug 6, 2021

@techninja1008 I'm pretty happy to see this as I was just hacking on the WASM plugin system and was trying to think of use cases which did not involve simply moving some of our current plugins into WASM modules.

I'll give this PR a review ASAP and see if I can comment on the design a bit.

@techninja1008
Copy link
Contributor Author

@thedodd I've tried to keep it purposefully simple but with scope to expand if needed. In particular, I've only added one point that hooks are called currently, but more can be easily added.

Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Overall, I think this is put together quite nicely. I think this is an excellent precursor to the plugin system.

Design Thoughts

Firstly, the simplicity of the design is great. Just provide the name of a target (like sh in this case), and arguments to be supplied.

  • Env vars which can be used in the hooks section, I like it, let's just pin down what they should be, and if we should add some additional env vars. Let's add docs for them as well.
  • Stages, I'm thinking that we actually have 3 stages: pre-build, build & post-build. The example in this PR would land in the post-build stage.
  • Long-term, we could add a custom stage which would have a name & allow for a list of before strings, where each before value would be a built-in or custom stage which must be executed prior to this custom stage being executed.
  • Maybe we should change the name of HookStage to PipelineStage so that we can use it for the plugin system as well when that lands (or maybe just add a comment about that :)).

Items To Do

  • We should add documentation on how this system should work.
  • The docs should probably be added to the Assets page of the site.
  • Perhaps some of the stage-related changes mentioned above and inline.

Long Term

We can probably have the plugin system work exactly the same way in terms of stages & when they are spawned.

Really excellent work here! I'm stoked to see how this evolves.

Comment on lines 2 to 4
name = "yew-example"
version = "0.1.0"
authors = ["Anthony Dodd <[email protected]>"]
Copy link
Member

Choose a reason for hiding this comment

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

Probably need to update the name here to yew-tailwindcss, and feel free to use your name as the author :).

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 updating the name to be in line with the other examples (ie. yew-tailwindcss-example). I'm just wondering if it's worth removing the authors field altogether as that's the direction convention seems to be moving in? (see rust-lang/cargo#9282) I've changed it to my name for now.

examples/yew-tailwindcss/Cargo.toml Outdated Show resolved Hide resolved
examples/yew-tailwindcss/index.html Outdated Show resolved Hide resolved
examples/yew-tailwindcss/src/main.rs Outdated Show resolved Hide resolved
examples/yew-tailwindcss/src/main.rs Outdated Show resolved Hide resolved
src/config/models.rs Outdated Show resolved Hide resolved
src/hooks.rs Outdated Show resolved Hide resolved
src/hooks.rs Show resolved Hide resolved
src/hooks.rs Show resolved Hide resolved
src/hooks.rs Outdated Show resolved Hide resolved
@techninja1008
Copy link
Contributor Author

I've just pushed a load of changes. I've tried to address every point here, including adding more env vars, modifying the hook (now called pipeline) stages, and documenting everything. Not yet added changelog entries, will do that when you're happy with it!

@thedodd thedodd mentioned this pull request Aug 8, 2021
@techninja1008
Copy link
Contributor Author

@thedodd just rebased to master & added changelog. Ready for review & merge if you're happy.

@techninja1008
Copy link
Contributor Author

@thedodd just resolved the outstanding merge conflicts

Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Well crafted PR! Thanks again @techninja1008. I'm going to merge this, and then fix the two small typo items & the CI linting issue from CI.

@@ -231,6 +232,20 @@ pub struct HashedFileOutput {
file_name: String,
}

/// A stage stage in the build process.
Copy link
Member

Choose a reason for hiding this comment

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

I'll de-dupe this stage stage after merge.

src/config/rt.rs Show resolved Hide resolved
site/content/assets.md Show resolved Hide resolved
- `TRUNK_STAGING_DIR`: the full path of the Trunk staging directory.
- `TRUNK_DIST_DIR`: the full path of the Trunk dist directory.
- `TRUNK_PUBLIC_URL`: the configured public URL for Trunk.

Copy link
Member

Choose a reason for hiding this comment

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

Top-notch docs right here. Thank you!

@thedodd thedodd merged commit 23a529b into trunk-rs:master Aug 20, 2021
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.

2 participants