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.

Sign up for GitHub

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

Jump to bottom

Fix for Issue 1418: [@s-ui/pde] use structured args in useFeature #1472

Closed
wants to merge 3 commits into from
Closed

Fix for Issue 1418: [@s-ui/pde] use structured args in useFeature #1472

wants to merge 3 commits into from

Conversation

theonly1me
Copy link

Fix for Issue 1418: [@s-ui/pde] use structured args in useFeature

Description

  • Update useFeature hook to accept structured args instead of raw args
  • Update unit tests to account for the changes to the useFeature hook
  • Update README.md to account for changes to the useFeature hook
  • Correct a spelling mistake in CONTRIBUTING.md (versionning -> versioning)

Related Issue

Fix #1418

Example

Example - 1

const MyComponent = () => {
  const {isActive} = useFeature({featureKey: 'myFeatureKey'}) // isActive = true when the feature flag is activated

  return <p>The feature 'myFeatureKey' is {isActive ? 'active' : 'inactive'}</p>
}

Example - 2

 useFeature({
              featureKey: 'featureKey3',
              queryString: '?suipde_featureKey3=off&suipde_feature1=on'
            })

@theonly1me
Copy link
Author

theonly1me commented Oct 4, 2022 •
edited
Loading

@rmoralp Can you please review this PR?
I've fixed this issue since I saw it tagged for hacktoberfest. Please feel free to let me know in case you guys are not looking for external contributors :)

@arnau-rius
Copy link
Contributor

I have approved workflow run from public fork 👍🏽

@arnau-rius
Copy link
Contributor

Looks like something was wrong during the build. Let's see if we can take a closer look of the issue in the next days.

@theonly1me
Copy link
Author

Thanks @arnau-rius, the following mocha test is failing:

1) copyfiles
       should copy files from src to dest and flatten it:
     Error: ENOENT: no such file or directory, copyfile 'fixturesCopy/a.js' -> 'runtime/a.js'

It is trying to copy the a.js from fixturesCopy directory to the runtime directory. This runtime directory seems to be getting generated during the build. It could be a random failure from looking at the logs and since I don't really have any changes here. Could you please re-trigger the build?

@rmoralp
Copy link
Contributor

rmoralp commented Oct 5, 2022

Could you make a commit type fix or feat in order to mark it as a breaking change?
It will trigger a major version of this package

@rmoralp
Copy link
Contributor

rmoralp commented Oct 5, 2022

BTW code looks great.

@theonly1me
Copy link
Author

Could you make a commit type fix or feat in order to mark it as a breaking change?
It will trigger a major version of this package

Alright, sure :)

BTW code looks great.

Thanks @rmoralp!

@theonly1me
fix: [@s-ui/pde] update useFeature hook to accept structured args ins…
751e6fc 
…tead of raw args

- In order to maintain coherency between the useExperiment and useFeature hooks, refactor the useFeature hook to accept structured args.
- Update unit tests
@theonly1me
docs: [@s-ui/pde] update README.md to account for refactoring of the …
f726a64 
…useFeature hook
@theonly1me
Copy link
Author

@rmoralp I have updated the commit type to fix. Could you please re-trigger the workflow build?

@rmoralp rmoralp added the hacktoberfest-accepted Accepted for hacktoberfest, merged or we will merge later label Oct 5, 2022
@rmoralp
Copy link
Contributor

rmoralp commented Oct 5, 2022

As I see, that commit names does not satisfy our conventional commit convention. You should use npm run co to make the commits

@theonly1me
Copy link
Author

theonly1me commented Oct 5, 2022 •
edited
Loading

@rmoralp, I see. I'll keep that in mind for any future PRs. Thanks. Please feel free to let me know in case you want me to make any further changes to the current PR.

@andresin87
Copy link
Member

@rmoralp this is a major. The version number must be (manual) updated also b4 merging if no commit message has it declared

@theonly1me theonly1me closed this Oct 5, 2022
@theonly1me
docs: [@s-ui/pde] correct spelling mistake in CONTRIBUTING.md
b5262af 
Correct the spelling from 'versionning' to 'versioning'
@theonly1me theonly1me reopened this Oct 5, 2022
@theonly1me
Copy link
Author

@rmoralp @andresin87 I've rebased my branch and fixed up the commits via the npm run co. The major version has been updated now. Please re-trigger the build workflow and feel free to let me know in case there's anything else you need me to do. Thanks.

@alextremp
Copy link
Collaborator

alextremp commented May 4, 2023 •
edited
Loading

Hi @theonly1me @andresin87 which is the current status of this PR?
In a short period of time we'll had to add a new feature adding a new parameter to the useFeature ( #1601 ) and would be nice to have the structured args already

@theonly1me theonly1me closed this Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kikoruiz kikoruiz Awaiting requested review from kikoruiz kikoruiz is a code owner

@rmoralp rmoralp Awaiting requested review from rmoralp

@sui-bot sui-bot Awaiting requested review from sui-bot sui-bot is a code owner

@arnau-rius arnau-rius Awaiting requested review from arnau-rius

@andresz1 andresz1 Awaiting requested review from andresz1 andresz1 is a code owner

Assignees

@theonly1me theonly1me

Labels
breaking-change hacktoberfest-accepted Accepted for hacktoberfest, merged or we will merge later not-ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[sui-pde] use structured args in useFeature
5 participants
@theonly1me @arnau-rius @rmoralp @andresin87 @alextremp