refactor: move argument methods out of the Command class #56
Conversation
|
I'm still not fan of Apart from that it sounds good to me - perhaps a test to ensure that the tree shaking works would help potential regression (for example if we change the build system later on). |
merceyz
force-pushed
the
merceyz/treeshake
branch
2 times, most recently
from
0a3cc9b
to
333b39f
Compare
Sounds better and I totally agree 👍
Test added 👍 |
tests/treeshake.test.ts
Outdated
| const code = await result.generate({ format: 'esm' }); | ||
|
|
||
| // @ts-expect-error - matchSnapshot is added by a plugin | ||
| expect(code.output[0].code.replace(/\r\n?/g, '\n')).to.matchSnapshot(this); |
There was a problem hiding this comment.
I'm not sure snapshots are the right tool here, they will be very susceptible to changes 🤔
There was a problem hiding this comment.
Updated the test to instead check the added size; should have lower noise but still catch treeshaking problems.
What's the problem this PR addresses?
It's currently impossible for rollup and webpack to treeshake methods from classes so even if you're only using
Command.Stringarguments all the other types will be included in the bundleHow did you fix it?
Move the argument methods out of the Command class