fix: bundle dependencies inline and fix es build #116
Conversation
rollup.config.js
Outdated
| @@ -25,8 +25,7 @@ function createEntry(options) { | |||
| input, | |||
| external: [ | |||
| 'vue', | |||
| 'lodash/mergeWith', | |||
| 'lodash/isString' | |||
| '@vue/compiler-dom' | |||
There was a problem hiding this comment.
Not needed in master right now, but will be very soon for this feature when merged, so it's fine to include it here.
|
Can we inline a cheap implementation of
For merge with: Even if we can't get rid of using a deep merge... can we at least write a one-liner reducer or inline a lighter weight deepMerge library that doesn't require ~20 helper files? |
|
Yep @JessicaSachs was just thinking the same thing. I tried this with vite and it works great. I will look into some more simple |
|
I will investigate. Good thing we uncovered this bug. |
| const wrapper2 = mount(Component, { props: { msg: 'Wrapper2' } }) | ||
| expect(wrapper1.text()).toEqual('Wrapper1 Hello world') | ||
| expect(wrapper2.text()).toEqual('Wrapper2 Hello world') | ||
| const HelloLocal = { |
There was a problem hiding this comment.
Should we add a test if the locally registered component is overwritten properly?
There was a problem hiding this comment.
We have this in the other test (I think?)
I will check.
|
@dobromir-hristov I guess - I just ran the tests in watch mode until they passed. Do we have some tests that use arrays? The mixin tests are passing, which I believe use arrays. Looks like we might need a tests for |
|
Yeah that plus should we deal with mixins somehow? Are array declaraitons, like mixins concatinated or overwritten? |
|
Pushed a test for the mixins, which proved me right unfortunately. Arrays should be concatinated, not merged. Overall we want a more specialized merge function.
|
|
thanks for that @dobromir-hristov, been a bit busy these last few days. I'll will look at this now. edit: yep, mutation is really the worst thing. I will fix this! |
|
Ok @dobromir-hristov getting closer, this is much cleaner and closer to what we had before - just a few failing tests to fix up. |
|
ok @dobromir-hristov we are green! Added all the tests. No more mutation :D PS couldn't get this one to pass: https://github.com/vuejs/vue-test-utils-next/pull/116/files#diff-9def4df294f4d44655f7400ace7b394bR220 If you have a global stub, is there really a use-case to provide a local, different stub? This seems like a unlikely edge case at best. Great review on this - found quite a few bugs (config.stubs was not actually working. We now have tests). Let me know if I missed any :) Excited to get this one merged up and released. |
|
I will check it out in the morning :) |
|
@dobromir-hristov we good to merge? thanks for refactor/tests. |
|
Think so |
As pointed about by @JessicaSachs, our es build was depending on cjs dependencies.
Although I like the idea of only shipping our code and asking the users to install dependencies like lodash, it does add a bit of complexity. Eg, depending if you are using VTU in a browser/node/es build, you need to include dependencies differently.
In this PR, we now inline some dependencies:
lodash/isString,lodash/mergeWitheventTypes(fortrigger)Vue and compiler-sfc and compiler-dom are still peer dependencies, so the user will bring their own version of those.
This does increase the bundle size - from 900 lines to around 4000. That said, mostly you will not be shipping this library to users, and 4000 lines is before minify, so it could be smaller, if the user wants to make it smaller, so I think this trade-off is probably fine. Simple is best!
For comparison, VTU v1 is around 10k lines (also not minified).