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

Migrate Frontity Connect to react easy state [21pt] #415

Merged
merged 39 commits into from
Apr 16, 2021

Conversation

luisherranz
Copy link
Member

@luisherranz luisherranz commented May 4, 2020

What:

Stop using our own fork and migrate back to react-easy-state.

Why:

Because they are adding hooks to their library, so we can build on top of it. This means that Frontity Connect will become an opinionated version of react-easy-state instead of a fork.

How:

I removed all our code, added react-easy-state back and used the hooks to replicate our changes.

Tasks:

Unrelated:

@luisherranz luisherranz self-assigned this May 4, 2020
@changeset-bot
Copy link

changeset-bot bot commented May 4, 2020

🦋 Changeset detected

Latest commit: a86e636

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@frontity frontity deleted a comment from sonarqubecloud bot May 4, 2020
@luisherranz luisherranz changed the title Migrate Frontity Connect to react easy state WIP: Migrate Frontity Connect to react easy state May 4, 2020
@luisherranz luisherranz marked this pull request as draft May 6, 2020 05:37
@luisherranz luisherranz changed the title WIP: Migrate Frontity Connect to react easy state Migrate Frontity Connect to react easy state May 6, 2020
@frontity frontity deleted a comment from sonarqubecloud bot Jun 26, 2020
This was referenced Jun 29, 2020
luisherranz added a commit that referenced this pull request Jul 1, 2020
TSDocs disabled by `@luisherranz` in this file because he needs to
figure out first which types will remain when we merge this PR
#415, taking into account that
`react-easy-state` and `@nx-js/observer-util` already have types
themselves.
luisherranz added a commit that referenced this pull request Jul 2, 2020
Because I want to refactor those types in the
#415 PR.
@luisherranz luisherranz linked an issue Jul 27, 2020 that may be closed by this pull request
3 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jul 31, 2020

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 8 8
Warnings 0 0
Ignored 0 N/A
  • Result: ❌ failure

  • Annotations: 8 total


[failure] jsdoc/require-param

Requires that all function parameters are documented.


[failure] jsdoc/require-jsdoc

Require JSDoc comments


Report generated by eslint-plus-action

@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #415 (a86e636) into dev (d0faec5) will decrease coverage by 1.49%.
The diff coverage is 96.37%.

Impacted Files Coverage Δ
packages/connect/src/create-store.js 91.67% <95.46%> (ø)
packages/connect/src/connect.js 75.37% <96.97%> (+10.43%) ⬆️

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2020

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 4 4
Warnings 0 0
Ignored 0 N/A
  • Result: ❌ failure

  • Annotations: 4 total


[failure] jsdoc/require-param

Requires that all function parameters are documented.


[failure] jsdoc/require-jsdoc

Require JSDoc comments


Report generated by eslint-plus-action

@luisherranz
Copy link
Member Author

I have updated the code to use the last alpha versions from react-easy-state and @nx-js/observer-util. I've also merged it with the new useConnect hook.

@luisherranz luisherranz marked this pull request as ready for review April 14, 2021 13:25
@luisherranz luisherranz requested a review from DAreRodz April 14, 2021 13:25
@luisherranz
Copy link
Member Author

This is finally ready for review 🙂

I've made a video to go over the changes: https://www.loom.com/share/e48c9accaabd4b69b8f4cc2349ed997e

@luisherranz luisherranz requested review from michalczaplinski and removed request for DAreRodz April 14, 2021 13:30
Copy link
Member

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

I haven't finished reviewing it all but I have some questions in the meantime 🙂

.changeset/forty-mayflies-joke.md Outdated Show resolved Hide resolved
packages/connect/src/__tests__/types.tests.tsx Outdated Show resolved Hide resolved
packages/connect/src/connect.js Outdated Show resolved Hide resolved
packages/connect/src/connect.js Show resolved Hide resolved
packages/connect/src/connect.js Show resolved Hide resolved
packages/connect/src/connect.js Show resolved Hide resolved
@luisherranz
Copy link
Member Author

Thanks for the reviews guys. I've made the changes and should be up for a review again 🙂

@github-actions
Copy link
Contributor

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Comment on lines +73 to +80
scheduler: () => {
// Trigger a new rerender if the component has already been
// mounted.
if (reaction.current.mounted) setState({});
// Annotate it as "changed" if the component has not been mounted
// yet.
else reaction.current.changedBeforeMounted = true;
},
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: The custom scheduler is used here in order to fix the bug: RisingStack/react-easy-state#196 (comment)

@michalczaplinski michalczaplinski self-requested a review April 16, 2021 02:12
@github-actions
Copy link
Contributor

github-actions bot commented Apr 16, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link
Member

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

OK, looks great @luisherranz 🙂 Let's merge this!

@luisherranz luisherranz merged commit 750a368 into dev Apr 16, 2021
@luisherranz luisherranz deleted the migrate-to-react-easy-state branch April 16, 2021 07:56
@luisherranz
Copy link
Member Author

Merged! Thanks a lot guys! 🙂

@SantosGuillamot SantosGuillamot mentioned this pull request Apr 26, 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.

Cursor jumps to end of input fields
3 participants