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

Support feature flags via CLI close #60 #183

Merged
merged 1 commit into from Jun 30, 2022

Conversation

Tanja-4732
Copy link
Contributor

@Tanja-4732 Tanja-4732 commented Jun 10, 2021

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.

This PR extends #166 to add data-cargo-all-features and data-cargo-no-default-features as well as CLI options for these build-config options.

Notice
This is (obviously) a work-in-progress.
I'll be using (and modifying) the work done in #166 and introduce --all-features and --no-default-features.
In a future PR, I may even add support for that via a <link data-trunk> tag (extending #166 to "all" and "no-default").

@Tanja-4732
Copy link
Contributor Author

@thedodd You'll have to add #60 to the "Linked issues" list of this PR, since I haven't created it (the issue) myself.

src/config/rt.rs Outdated Show resolved Hide resolved
src/config/rt.rs Outdated Show resolved Hide resolved
src/pipelines/rust_app.rs Outdated Show resolved Hide resolved
@Tanja-4732 Tanja-4732 marked this pull request as ready for review June 10, 2021 20:46
@Tanja-4732
Copy link
Contributor Author

I'll update the CHANGELOG and the README after the code-changes are approved.

Once that's done I'll squash my commits.

@Tanja-4732
Copy link
Contributor Author

Tanja-4732 commented Jun 10, 2021

@thedodd What do you mean by the following?

I want to add -Z as well, however that has a slightly more complex command structure, and it might be best to leave that off for now as extra credit.

see #60 (comment)

@Tanja-4732 Tanja-4732 marked this pull request as draft July 30, 2021 00:07
@Tanja-4732
Copy link
Contributor Author

I converted this back to a draft to work on the conflicts in the files

  • src/config/mod.rs
  • src/config/rt.rs
  • src/pipelines/rust_app.rs

Btw, @thedodd, what do you mean with

I want to add -Z as well, however that has a slightly more complex command structure, and it might be best to leave that off for now as extra credit.

As in, what would -Z do? For what would it be an alias? See #60 (comment)

@Tanja-4732 Tanja-4732 marked this pull request as ready for review August 1, 2021 14:22
@Tanja-4732 Tanja-4732 force-pushed the support_feature_flags_issue-60 branch from fb2f022 to 0dd4414 Compare May 17, 2022 17:23
@Tanja-4732
Copy link
Contributor Author

Tanja-4732 commented May 17, 2022

This PR addressing #60 is now again ready to be reviewed.

Once @thedodd approves (or I fix all things he'd like me to change and then approves), I'll squash the commits and #60 can be closed by merging this.

@Twister915
Copy link

Hey! Is there any update on this?I'd love to use this feature, so I can reduce my wasm binary size (using build-std).

@Tanja-4732
Copy link
Contributor Author

Tanja-4732 commented Jun 15, 2022

Hey! Is there any update on this?I'd love to use this feature, so I can reduce my wasm binary size (using build-std).

I'll try to get this PR up-to-date with master in the next 24h.
I've done this a couple of times now, but it's never been merged so far.

@tiye
Copy link

tiye commented Jun 15, 2022

ping @thedodd @dnaka91

@Tanja-4732
Copy link
Contributor Author

Tanja-4732 commented Jun 15, 2022

I'm waiting for a quick go-ahead, then I'll get to the checklist of

  • 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.

I want to know if the way I implemented this makes sense for the project.

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.

One small item. We should be g2g after that.

BTW, the -Z bit that I was referring to is in reference to Cargo's -Z, which passes options directly to rustc. Let's not tackle that in this PR.

#[clap(long)]
#[serde(default)]
pub all_features: bool,
/// The public URL from which assets are to be served [default: /]
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the docstring needs to be updated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new docstring:

/// A comma-separated list of features to activate, must not be used with all_features [default: ""]
#[clap(long)]
pub features: Option<String>,

The new output for trunk build -h:

trunk-build 
Build the Rust WASM app and all of its assets

USAGE:
    trunk build [OPTIONS] [TARGET]

ARGS:
    <TARGET>    The index HTML file to drive the bundling process [default: index.html]

OPTIONS:
        --all-features               Build with all features [default: false]
    -d, --dist <DIST>                The output dir for all final assets [default: dist]
        --features <FEATURES>        A comma-separated list of features to activate, must not be
                                     used with all-features [default: ""]
        --filehash <FILEHASH>        Whether to include hash values in the output file names
                                     [default: true]
    -h, --help                       Print help information
        --no-default-features        Build without default features [default: false]
        --public-url <PUBLIC_URL>    The public URL from which assets are to be served [default: /]
        --release                    Build in release mode [default: false]

Let me know if this works for you/the team;
I'll get to writing the changelog & readme stuff in the meantime.

src/config/rt.rs Show resolved Hide resolved
@Tanja-4732
Copy link
Contributor Author

Tanja-4732 commented Jun 17, 2022

I just pushed changes to CHANGELOG.md and to assets.md to reflect changes made to data-cargo attributes - see commit 1b26c12 for details.

Two things:

  1. Please let me know when to squash EDIT: squashed.
  2. Should we introduce tests for this functionality?

@Tanja-4732 Tanja-4732 requested a review from thedodd June 17, 2022 13:23
@Tanja-4732 Tanja-4732 force-pushed the support_feature_flags_issue-60 branch from a84451c to 744bad5 Compare June 17, 2022 16:04
@Tanja-4732
Copy link
Contributor Author

Tanja-4732 commented Jun 17, 2022

I've just squashed the commits on my branch support_feature_flags_issue-60 into one commit: 744bad5.
The old commits are still available on support_feature_flags_issue-60_no-squash, in case somebody wants to see them.

I now consider this ready-to-merge; awaiting feedback.

@Tanja-4732
Copy link
Contributor Author

Tanja-4732 commented Jun 24, 2022

Friendly reminder for @thedodd

This is ready-to-merge see #183 (comment) 🚀:

  • (1st round of ) Requested changes implemented
  • Changelog amended for new features
  • Blog post explaining the features amended
  • Squashed down into one commit

Please let me know if you'd like me to make further changes.

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.

Woot woot! Thanks for all of the hard work and patience on this @Bernd-L!

@thedodd thedodd merged commit b291dc2 into trunk-rs:master Jun 30, 2022
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.

None yet

4 participants