avajs / typescript

Test TypeScript projects using AVA.
MIT License
73 stars 16 forks source link

Remove is-plain-object dependency #13

Closed Conaclos closed 4 years ago

Conaclos commented 4 years ago

Hi!

This PR removes is-plain-object dependency and so its indirect dependency is-object.

Motivations:

novemberborn commented 4 years ago
  • There is no particular reasons to reject no-plain objects for config objects.

Well, there is: to encourage folks keep their configuration "plain", as it were.

  • Less dependencies

It's still a dependency in AVA, so it doesn't make a difference.

Conaclos commented 4 years ago

Well, there is: to encourage folks keep their configuration "plain", as it were.

I agree. However, in my viewpoint, it is not worth two dependencies. The choice (2) (see commit message) could be the best option, but it seems hacky.

It's still a dependency in AVA, so it doesn't make a difference.

Yes it is also a dependency of @ava/babel. If this PR is accepted, I plan to ship similar PRs to these projects. Ava has a lot of dependencies, it could be great to reduce that. If you are not ok with this change, that's ok. I do not take this personally :)

novemberborn commented 4 years ago

see commit message

You're hiding all the insight!

  1. typeof config === 'object' && config !== null

This allows arrays.

  1. config != null && config.constructor === Object

Yes this is awkward, but mostly because config.constructor can be modified.

  1. config instanceof Object (3) excludes prototype-less objects - i.e. Object.create(null). This is certainly fine in our case. However this is a bit confusing from an API perspective to reject these objects.

is-plain-object doesn't allow these objects either. instanceof Object allows any class instance, however.

Instead we can use Reflect.getPrototypeOf():

function isPlainObject (x) {
  return x !== null && typeof x === 'object' && Reflect.getPrototypeOf(x) === Object
}

I'd be happy with that — ideally it'd be in a package but it's fine.

it is not worth two dependencies.

FWIW, minimizing the number of dependencies isn't a goal of these libraries.

Conaclos commented 4 years ago

I applied the changes. I fixed an issue in your function: the prototype of x must be compared to the prototype of Object (not Object itself).

FWIW, minimizing the number of dependencies isn't a goal of these libraries.

Thus we can rely on lodash isPlainObject function. Ava has already some dependencies on lodash.

novemberborn commented 4 years ago

Thanks @Conaclos!