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

🚧 Remove lodash from immutadot package #143

Merged
merged 51 commits into from
Dec 7, 2017
Merged

Conversation

frinyvonnick
Copy link
Contributor

Prerequisites

Description

Remove lodash from immutadot package

Issue : #78

@frinyvonnick frinyvonnick changed the title 🔨 Remove lodash from fill Remove lodash from immutadot package Nov 28, 2017
@codecov-io
Copy link

codecov-io commented Nov 28, 2017

Codecov Report

Merging #143 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #143   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          76     77    +1     
  Lines         252    280   +28     
=====================================
+ Hits          252    280   +28
Impacted Files Coverage Δ
...kages/immutadot-lodash/src/util/lodashFpConvert.js 100% <ø> (ø)
packages/immutadot-lodash/src/array/remove.js 100% <ø> (ø) ⬆️
packages/immutadot-lodash/src/array/pullAllWith.js 100% <ø> (ø) ⬆️
packages/immutadot-lodash/src/array/pull.js 100% <ø> (ø) ⬆️
...kages/immutadot-lodash/src/util/convertLodashFp.js 100% <ø> (ø)
packages/immutadot-lodash/src/object/merge.js 100% <ø> (ø) ⬆️
packages/immutadot/src/core/set.js 100% <ø> (ø) ⬆️
packages/immutadot-lodash/src/array/pullAllBy.js 100% <ø> (ø) ⬆️
packages/immutadot-lodash/src/array/pullAll.js 100% <ø> (ø) ⬆️
packages/immutadot-lodash/src/object/defaults.js 100% <ø> (ø) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbeda1d...a85d358. Read the comment docs.

@nlepage nlepage added this to the 1.0.0-RC1 milestone Nov 28, 2017
@nlepage nlepage changed the title Remove lodash from immutadot package 🚧 Remove lodash from immutadot package Nov 28, 2017
@frinyvonnick frinyvonnick changed the base branch from feature/78 to master November 28, 2017 20:38
@frinyvonnick frinyvonnick force-pushed the refactore/remove-lodash branch 2 times, most recently from 937021f to c3a0d6a Compare November 28, 2017 21:06
@nlepage nlepage mentioned this pull request Nov 29, 2017
4 tasks
@@ -0,0 +1,10 @@
import { convert } from './convert'
Copy link
Member

Choose a reason for hiding this comment

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

This import is undefined, only convertLodashFp is exported.
I think no index is necessary for util namespace in immutadot-lodash...

@@ -2,3 +2,4 @@ export * from './array'
export * from './collection'
export * from './object'
export * from './string'
export * from './util'
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this should be exported

import map from 'lodash/map'
import mapValues from 'lodash/mapValues'
import omit from 'lodash/omit'
const isSymbol = sym => typeof sym === 'symbol'
Copy link
Member

Choose a reason for hiding this comment

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

This one exists in util/lang

@@ -45,7 +53,7 @@ class ChainWrapper {
* @since 0.1.11
*/
_absolutePath(path) {
return concat(toPath(this._path), toPath(path))
return [].concat(unsafeToPath(this._path), unsafeToPath(path))
Copy link
Member

Choose a reason for hiding this comment

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

unsafeToPath is guaranted to return an array, and concat doesn't mutate the array, so you can do unsafeToPath(this._path).concat(unsafeToPath(path))


const head = arr => arr[0]

const drop = (arr, n = 1) => arr.slice(n)
Copy link
Member

Choose a reason for hiding this comment

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

Inline this one

const args = concat(
map(this._paths, usingPath => {
const args = [].concat(
this._paths.map(usingPath => {
Copy link
Member

Choose a reason for hiding this comment

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

this._paths is guaranted to be an array, and concat doesn't mutate, call this._paths.concat

@@ -34,7 +36,7 @@ class ProtectHandler {
get(target, property) {
const reference = this._peek()[property]
if (!isObject(reference)) return reference
return new Proxy(reference, new ProtectHandler(this.chainWrapperRef, concat(this.path, property)))
return new Proxy(reference, new ProtectHandler(this.chainWrapperRef, [].concat(this.path, property)))
Copy link
Member

Choose a reason for hiding this comment

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

Same remark

@@ -46,7 +48,7 @@ class ProtectHandler {
* @since 0.3.0
*/
set(target, property, value) {
this.chainWrapperRef.chainWrapper = this.chainWrapperRef.chainWrapper.set(concat(this.path, property), value)
this.chainWrapperRef.chainWrapper = this.chainWrapperRef.chainWrapper.set([].concat(this.path, property), value)
Copy link
Member

Choose a reason for hiding this comment

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

Same remark

@@ -58,7 +60,7 @@ class ProtectHandler {
* @since 0.3.0
*/
deleteProperty(target, property) {
this.chainWrapperRef.chainWrapper = this.chainWrapperRef.chainWrapper.unset(concat(this.path, property))
this.chainWrapperRef.chainWrapper = this.chainWrapperRef.chainWrapper.unset([].concat(this.path, property))
Copy link
Member

Choose a reason for hiding this comment

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

Same remark

@@ -60,7 +68,7 @@ class ChainWrapper {
return new ChainWrapper(
this._wrapped,
this._path,
concat(this._flow, object => fn(object, this._absolutePath(path), ...args)),
[].concat(this._flow, object => fn(object, this._absolutePath(path), ...args)),
Copy link
Member

Choose a reason for hiding this comment

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

We only add one element, I think we can use this._flow.push

@nlepage nlepage merged commit 79d0b59 into master Dec 7, 2017
@nlepage nlepage deleted the refactore/remove-lodash branch December 7, 2017 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants