search

adopted-ember-addons / ember-file-upload

File uploads for Ember apps
https://ember-file-upload.pages.dev/
Other
201 stars 119 forks source link

Move @embroider/macros to peerDeps #1030

Closed RobbieTheWagner closed 6 months ago

RobbieTheWagner commented 11 months ago

I am not sure if this actually helps or not, but we noticed a lot of lodash being included in our app after installing ember-file-upload and my hypothesis is @embroider/macros should be a peerDep since it is just used in the mirage stuff and that the lodash is coming from it, but I could be totally wrong.

gilest commented 11 months ago

Hmm I'm not sure about this one... In the readme for that package it explicitly says to include in dependencies.

Agree that apps which don't use mirage won't need it but it being in the dep tree shouldn't have any negative affects.

we noticed a lot of lodash being included in our app after installing ember-file-upload

If that's the case then this is an issue with @embroider/maros, ember-auto-import or Embroider itself. We may need to investigate further and raise a bug where relevant.

Are you using the mirage handler in your app? Are you using embroider or classic build?

jelhan commented 11 months ago

we noticed a lot of lodash being included in our app after installing ember-file-upload

Did you noticed it in development or production build? If I recall correctly, Embroider macros work differently for development and production builds.

RobbieTheWagner commented 11 months ago

we noticed a lot of lodash being included in our app after installing ember-file-upload

Did you noticed it in development or production build? If I recall correctly, Embroider macros work differently for development and production builds.

No build at all, just installing ember-file-upload added a ton of lodash to deps in our pnpm lock.

RobbieTheWagner commented 11 months ago

Hmm I'm not sure about this one... In the readme for that package it explicitly says to include in dependencies.

Agree that apps which don't use mirage won't need it but it being in the dep tree shouldn't have any negative affects.

we noticed a lot of lodash being included in our app after installing ember-file-upload

If that's the case then this is an issue with @embroider/maros, ember-auto-import or Embroider itself. We may need to investigate further and raise a bug where relevant.

Are you using the mirage handler in your app? Are you using embroider or classic build?

We're not using embroider, just classic build.

jelhan commented 11 months ago

No build at all, just installing ember-file-upload added a ton of lodash to deps in our pnpm lock.

Why is that a problem?

RobbieTheWagner commented 11 months ago

No build at all, just installing ember-file-upload added a ton of lodash to deps in our pnpm lock.

Why is that a problem?

Because they are deps and added to the bundle. If they were devDeps, it would be no problem.

jelhan commented 11 months ago

Because they are deps and added to the bundle.

Not all code of installed dependencies for into the bundle. Actually only a very small subset does. If everything works as expected, @embroider/macros should not push any code into production build. But it is expected to be installed. It is needed at build-time.

Please check if it affects the production bundle or not. It's only a problem of unexpected code is pushed I to the production build.

RobbieTheWagner commented 11 months ago

Because they are deps and added to the bundle.

Not all code of installed dependencies for into the bundle. Actually only a very small subset does. If everything works as expected, @embroider/macros should not push any code into production build. But it is expected to be installed. It is needed at build-time.

Please check if it affects the production bundle or not. It's only a problem of unexpected code is pushed I to the production build.

It's weird that all the lodash things went from being devDeps to deps when we installed ember-file-upload though. I consider that incorrect if they aren't going into the build.