Skip to content

Conversation

@43081j
Copy link

@43081j 43081j commented Aug 1, 2020

You really don't need this micro-dependency, Array.isArray will do just fine for your use case here.

One less dependency, and now zero-dependencies overall.

im aware you were trying to remove array support a few years back but that didn't seem to go anywhere. this package is still widely used so would be nice to strip one more unnecessary bit of cruft from the dependency tree regardless.

@Qix-
Copy link
Owner

Qix- commented Aug 4, 2020

There were reasons back then for including it, fwiw. Array support just needs to be removed entirely and a new major needs to be released.

@43081j
Copy link
Author

43081j commented Aug 4, 2020

Is that any time soon? If not, we could still merge this now as a patch version until that happens.

@Qix-
Copy link
Owner

Qix- commented Aug 4, 2020

Because this isn't a patch change. This is a major break. No matter how seemingly inconsequential, changing to Array.isArray() is not the same behavior as is-arrayish.

Is that any time soon?

Whenever someone would like to file a PR - I don't have time right now.

@43081j
Copy link
Author

43081j commented Aug 4, 2020

Yep that's true, though it should have never had the dependency in the first place.

I'll do it.

@Qix-
Copy link
Owner

Qix- commented Aug 4, 2020

though it should have never had the dependency in the first place.

I understand that now that is the case, but when this module was released, it was not. There were still packages that returned Array-like objects that I wanted to support but that did not meet Array.isArray()'s requirements (did not have Array as the __proto__, etc.).

One of the worst offenders was (and still is) the DOM.

Please don't assume malice in code.

@43081j
Copy link
Author

43081j commented Aug 4, 2020

here.

i read your README and this is exactly the behaviour your own docs specify (which in fact, the issue from years ago and the proposed diff did not support and actually would've broken).

as the readme describes, something like this:

var AdvancedError = errorEx('AdvancedError', {
	foo: {
		message: function (value) {
			if (value) {
				return 'bar ' + value;
			}
			return null;
		}
	}
})

would now result in bar theValueOfFoo, whereas if foo isn't set, it would result in whatever .message was.

did it in the same pr because why not, its tiny either way and the same objective.

@43081j
Copy link
Author

43081j commented Sep 8, 2020

@Qix- what do you wanna do about this PR? it maintains your current interfaces/functionality as far as i can see

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.

2 participants