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

Initialization script customization #184

Merged
merged 14 commits into from
Sep 20, 2021

Conversation

wcrbrm
Copy link
Contributor

@wcrbrm wcrbrm commented Jun 11, 2021

Motivation:

The intention behind this PR is to have full control over the script that initialized application and use trunk in more cases.
While in current version initialization templates are hardcoded, initialization may differ for other frameworks
and require more flexibility for debugging see Sauron SSR as example.

Changes:

  • Added the optional pattern_script field to the Trunk.toml for overloading the template of initialization script.
  • Added the optional pattern_preload field to the Trunk.toml for overloading the template of WASM preloading.
  • Added the optional pattern_params field to the Trunk.toml for extending pattern_script and pattern_preload with additional values, even including external files.
  • Parameters can be set up only in Trunk.toml and not from environment/CLI because they are typically multiline.

Backward compatibility is kept

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md 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

@wcrbrm hello there. Thanks for opening this PR. I can definitely see how there is value in being able to customize the JS init script used to load the WASM app. I'll circle back and give this a more in-depth review after v0.13 is released.

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.

@wcrbrm thanks for opening this PR! I apologize for how it has taken me to get around to reviewing this.

High-level, I like the approach. Seems quite flexible. Long-term, I think we should create a plugin which allows for full visibility of the manifest and all other outputs of the system. However, the plugin system is not ready yet, so this is quite good.

I have a few comments below about subtle changes which I think would be good. Let me know what you think.

  • Before we merge, we need to add documentation on this feature, and the place to do that is the site folder. I would say that this should be added to the assets page.

src/pipelines/rust_app.rs Show resolved Hide resolved
src/config/models.rs Outdated Show resolved Hide resolved
src/config/models.rs Outdated Show resolved Hide resolved
src/pipelines/rust_app.rs Outdated Show resolved Hide resolved
src/pipelines/rust_app.rs Outdated Show resolved Hide resolved
src/pipelines/rust_app.rs Outdated Show resolved Hide resolved
src/pipelines/rust_app.rs Outdated Show resolved Hide resolved
src/config/rt.rs Show resolved Hide resolved
src/config/models.rs Outdated Show resolved Hide resolved
wcrbrm and others added 10 commits August 20, 2021 19:43
Co-authored-by: Anthony Dodd <2380740+thedodd@users.noreply.github.com>
Co-authored-by: Anthony Dodd <2380740+thedodd@users.noreply.github.com>
Co-authored-by: Anthony Dodd <2380740+thedodd@users.noreply.github.com>
Co-authored-by: Anthony Dodd <2380740+thedodd@users.noreply.github.com>
Co-authored-by: Anthony Dodd <2380740+thedodd@users.noreply.github.com>
Co-authored-by: Anthony Dodd <2380740+thedodd@users.noreply.github.com>
Co-authored-by: Anthony Dodd <2380740+thedodd@users.noreply.github.com>
Co-authored-by: Anthony Dodd <2380740+thedodd@users.noreply.github.com>
@wcrbrm
Copy link
Contributor Author

wcrbrm commented Aug 20, 2021

Thanks for detailed review and putting proper wording in docs!
Branch in its current state was checked and it works for the cases I have

Do you want me to modify site/content/assets.md page to describe it or you will do it?
I actually think these parameters refer to site/content/configuration.md, as they are not actually referring to assets, and available only in configuration file Trunk.toml

@wcrbrm wcrbrm requested a review from thedodd August 20, 2021 19:22
@thedodd
Copy link
Member

thedodd commented Sep 20, 2021

@wcrbrm no worries. I'll update the site. Thank you!

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.

Awesome! Merging.

Comment on lines +34 to +44
/// Optional pattern of the script to be injected instead of standard [default: None]
/// Should include {base}, {wasm}, {js}
pub pattern_script: Option<String>,
/// Optional pattern of the preload links to be injected instead of standard [default: None]
/// Should include {base}, {wasm}, {js}
pub pattern_preload: Option<String>,
/// Can be read only from config file
/// Optional parameters of replacements inside
/// While {var} is being replaced with the provided value,
/// {@path} is replaced with contents of the provided file.
/// This allows insertion of some big JSON state or even HTML files
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to make a small tweak to this once I merge.

@thedodd thedodd merged commit 01c17af into trunk-rs:master Sep 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