Skip to content

Object mutation #2

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

Closed
miljan-aleksic opened this issue Nov 25, 2017 · 9 comments
Closed

Object mutation #2

miljan-aleksic opened this issue Nov 25, 2017 · 9 comments
Labels

Comments

@miljan-aleksic
Copy link

Hi @alexsasharegan,

in my local tests I am getting this strange behavior where the passed in objects are being mutated.
Example

const def = {on: { event }}
const def2 = {on: { event2 }}

const data = dataMerge(data, def, def2)

console.log(def) // { on: [event, event2] }

Before making some deeper tests and solutions I wanted to expose this issue and see if there is any reason behind.

Thank you.

@alexsasharegan
Copy link
Owner

alexsasharegan commented Nov 25, 2017

Good catch. The first argument passed to the function gets copied into a fresh target object, but it's a shallow copy. I'll work on a fix today.

@miljan-aleksic, would you be able to post a minimum reproducible scenario? I'm going to add testing for the function and I'd like to add a test tagged with this issue number.

Update: You can view the tests to see if your case is covered sufficiently.

alexsasharegan added a commit that referenced this issue Nov 26, 2017
BREAKING CHANGE: default export has been removed in favor of named export

BREAKING CHANGE: right-most arguments were not originally given precedence in all cases. This has been fixed.
@alexsasharegan
Copy link
Owner

alexsasharegan commented Nov 26, 2017

This has been fixed in v2. Consult the changelog for info on upgrading. Basically you just need to use named imports now, and everything else works.

Also, thanks for the mention on the vuejs repo (vuejs/vue#6895), @miljan-aleksic! I would love for this to be exposed from vue itself.

@miljan-aleksic
Copy link
Author

Awesome! The tests were absolutely necessary and thanks for the fix!!

Although, I wasn't able to test it as the compiled dist code is not converted to ES5 entirely. There is a spread operator present at function mergeData(...e).

Yes, hopefully, Vuejs team will take it into serious consideration. Thanks for your hard work!

@alexsasharegan
Copy link
Owner

Hmm... I'll figure out how the transpilation failed. Fix coming shortly!

@alexsasharegan
Copy link
Owner

I let the function type signature in the live function. My bad! All fixed.

@miljan-aleksic
Copy link
Author

Sorry, I got confused about the real issue, although I'm glad we found another one meantime :D

I am recompiling your code into my core lib using rollup (that way I don't add more dependencies) and I am getting this error from rollup CompileError: for...of statements are not supported..

I checked the dist quickly and saw the spread and assumed... but the issue was actually the for...of. That was my bad :)

@alexsasharegan
Copy link
Owner

Ah, looks like I tweaked the wrong tsconfig.json setting. Flipped that back to es5 to transpile the for of statements.

You don't need to compile this one from source if you don't want to since it holds no dependencies. But if you do, you'll need to wire up typescript with a similar config.

@miljan-aleksic
Copy link
Author

Actually, I am just reexporting, not recompiling and now is working as expected. The objects are not being mutated, the code is well transpiled, perfect, thank you so much!

@alexsasharegan
Copy link
Owner

Excellent! Thanks for being a patient beta tester!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants