Skip to content
This repository has been archived by the owner on Nov 12, 2017. It is now read-only.

Initial array prefixing implementation #14

Closed
wants to merge 1 commit into from

Conversation

rtsao
Copy link
Contributor

@rtsao rtsao commented May 31, 2016

Here's my first pass at fixing https://github.com/rofrischmann/inline-style-prefix-all/issues/13

One question: Is it intentional that this module mutates the original style object? I think an immutable API might be easier to reason about (e.g. debugging, etc.). I've implemented this feature in an immutable fashion, though I might be able to make this a bit simpler by using mutation. I suppose performance could be better with an API that mutates the original object.

@robinweser
Copy link
Owner

robinweser commented May 31, 2016

Hmm. Actually I never really thought about that much. I guess if you'd like to keep the original style object (I do not see any valid reason right now) you'd probably Object.assign({}, style) yourself before.
I think it might have performance issues if you're doing a lot of dynamic and periodic prefixing. Also I like that it's small and easy to understand as I wanted to keep this package as small and simple as possible, but of course I will think about that.

My suggestion might still be to just do:

const prefixedStyle = prefixAll(Object.assign({}, style))

prefixedStyle !== style

According the array prefixing itself: I will find some time tomorrow to check that in depth. I am actually too tired to do so.

@rtsao
Copy link
Contributor Author

rtsao commented May 31, 2016

That makes sense.

The usage example makes it seem like it might return a new style object (rather than mutating the original), which caught me by surprise. To keep the original object, the user would need to make a deep copy, I think Object.assign won't work. But you're right, there doesn't really seem to be a use case for an immutable API so I think that is fine.

I'll give this another go with a mutation in mind; I can probably simplify the map/reduce logic.

@robinweser
Copy link
Owner

I thought Object.assign would actually really clone the object, but yeah one might use object-deep-assign I guess. I really appreciate your work here :)

@rtsao
Copy link
Contributor Author

rtsao commented Jun 8, 2016

Closing in favor of: #16

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants