search

elwayman02 / ember-sinon-qunit

Sinon sandbox test integration for QUnit
MIT License
56 stars 30 forks source link

Test packages getting included in prod bundle chunk #891

Closed vstefanovic97 closed 10 months ago

vstefanovic97 commented 11 months ago

Looks like after bumping ember-sinon-qunit from 6.0.0 to 7.0.0 we are seeing sinon, qunit and some other packages getting included in one of ember-auto-import generated chunks for production builds.

I was able to reproduce this https://github.com/vstefanovic97/ember-sinon-qunit-bug-reproduction, repository is new ember app with just ember-sinon-qunit added.

Steps to reproduce

git clone https://github.com/vstefanovic97/ember-sinon-qunit-bug-reproduction
npm i
npm run build

After build if we inspect dist/index.html and chunks included we will see that one of the chunks contains sinon and qunit code.

After investigation it seems that this is the root cause https://github.com/elwayman02/ember-sinon-qunit/blob/master/index.js#L14-L30

Something we are doing here is not sitting well with ember-auto-import and because of that it includes test deps in prod bundle. Since preprocessJs is a private method we probably shouldn't be using it in the first place.

I propose just removing this code and having users import setupSinon as

import setupSinon from 'ember-sinon-qunit/test-support';
elwayman02 commented 11 months ago

Hmm, this would be a breaking change for the library, so I don't want to jump into this change aggressively. It'd be nice to understand better if there's an alternative that still allows this to work. It's weird that something we're doing in a hook for test support is impacting the prod bundle...

vstefanovic97 commented 11 months ago

@elwayman02 one way I think we can keep import as is would be to convert addon to v2, but that would also be a breaking change I think? Would you be more open to major release in that case, if we converted the addon to v2?

elwayman02 commented 11 months ago

@vstefanovic97 I think I'd be more amenable to a v2 conversion since that's an expected evolution of Ember addons moving forward. It would be easier to justify the breaking release at that point, given we're aligning with the future of Ember packages.

SergeAstapov commented 11 months ago

@elwayman02 @vstefanovic97 I would recommend work on v2 conversion in pieces (convert to monorepo, extracttest app, convert to v2, update import path, etc.) instead of single PR as it makes things simpler to review and grasp.

@elwayman02 would you like to add me as contributor to this addon to spread the maintenance work? I have limited bandwidth but would be happy to help with v2 conversion and some maintenance.

elwayman02 commented 11 months ago

@SergeAstapov sure, I'd be happy to have the contribution!

SergeAstapov commented 11 months ago

@elwayman02 @vstefanovic97 just realized we don't need a breaking change for v2 conversion as we even won't need to change import path.

as this addon already depends on ember-auto-import v2, conversion to v2 format is a non-breaking change.

we would then leave import path "as is" as it would be responsibility of ember-auto-import to include this addon to test only bundle as it would be used only in tests and tehre won't be any treeForAddonTestSupport at all

elwayman02 commented 11 months ago

Even better!

elwayman02 commented 10 months ago

Please test 7.4.0 and let me know if the issue is resolved!

SergeAstapov commented 10 months ago

yes it was resolved!