search

FormidableLabs / freactal

Clean and robust state management for React and React-like libs.
MIT License
1.65k stars 46 forks source link

removes dependencies #81

Closed Jascha-Sundaresan closed 7 years ago

Jascha-Sundaresan commented 7 years ago

This removes all external dependencies. :)

prop-types works as a devDep and lodash's usage was replaced w/ Object and typeof

The maintainers of this repo require that all pull request submitters adhere to the following:

The maintainers of this repository require you to select the semantic version type that the changes in this pull request represent. Please select one of the following:

maintainerd[bot] commented 7 years ago

maintainerd logging is enabled for this repository.

All actions related to rules and their enforcement will be logged here as a permanent record.

Click to view log...

- `2017-10-29T19:50:53.902Z:8a5c46c`: The pull request was created - `2017-10-29T19:51:53.550Z:f362552`: @Jascha-Sundaresan checked `I have read and will comply with the [contribution guidelines](https://github.com/FormidableLabs/freactal/blob/master/CONTRIBUTE.md).`. - `2017-10-29T19:51:55.378Z:f362552`: @Jascha-Sundaresan checked `I have read and will comply with the [code of conduct](https://github.com/FormidableLabs/freactal/blob/master/CONTRIBUTE.md).`. - `2017-10-29T19:52:08.160Z:f362552`: @Jascha-Sundaresan checked `All related documentation has been updated to reflect the changes made. _(required)_`. - `2017-10-29T19:52:10.461Z:f362552`: @Jascha-Sundaresan checked `My commit messages are cleaned up and ready to merge. _(required)_`. - `2017-10-29T19:52:12.271Z:f362552`: @Jascha-Sundaresan selected `patch` as the semantic version.

ryan-roemer commented 7 years ago

For server-side rendering isn't prop-types required? You'll get an import error if not (unless somehow the babel transformation to lib is removing require("prop-types") or something).

I've always seen it as at least a dependencies or peerDependencies.

Jascha-Sundaresan commented 7 years ago

My mistake. You're right.

I just confirmed w/ the prop-types documentation and they recommend leaving it as a dependency.

Rebasing my commit now.

ryan-roemer commented 7 years ago

LGTM.

@divmain -- You good with this one? Also looks like maintainerd is on the fritz again for the body check (?)

stefvhuynh commented 7 years ago

@ryan-roemer, @divmain, this LGTM.

ryan-roemer commented 7 years ago

LGTM. Closing and immediately reopening to kick maintainerd which seems on the fritz and stuck again.

ryan-roemer commented 7 years ago

@Jascha-Sundaresan -- Can you update to latest master and ping us again? Thanks!

divmain commented 7 years ago

I might want to keep the lodash implemention of assign here. Unless something has changed with recent versions of V8, the lodash implementation is still faster than Object.assign. Even if performance has improved, there's nothing really to lose in using lodash since this is server-side code, and I want a high-quality experience in older Node versions if its a freebie. Same thing with keys.

The other clean-up looks fine to me.

@jdalton, is this still the case?

jdalton commented 7 years ago

I'm 👍 on using builtin Object.assign.

divmain commented 7 years ago

Sweet, thanks JD. I'll merge this in.