search

symfony / stimulus-bridge

Stimulus integration bridge for Symfony projects
https://symfony.com/ux
75 stars 15 forks source link

removing class-properties #16

Closed weaverryan closed 3 years ago

weaverryan commented 3 years ago

See #11.

tl;dr we do not actually depend on this library, though Stimulus requires it if you're using its normal `targets = syntax. However, how that currently works with Encore is a bit of a mystery.

Still, I do not believe having this as a dependency solves or does anything. And since it's not part of any release yet, let's remove it.

tgalopin commented 3 years ago

Hum, I'm not sure, as it's used in the recipe

weaverryan commented 3 years ago

LOL. So THAT is how/why this works out-of-the-box 🤦

Here's what I think we should do:

A) Remove this as a dependency here. It is not really a dependency B) In Encore, we bump the dependency to require @babel/preset-env at least 7.10

... and that's all. The plugin would still be added directly in the user's project in webpack.config.js and it would be present thanks to preset-env.

weaverryan commented 3 years ago

This was closed unintentionally - my bad!

tgalopin commented 3 years ago

Should we update the recipe before doing that?

weaverryan commented 3 years ago

@tgalopin I decided not to put the package into the recipe, but just to make sure that it's available indirectly via Encore - in symfony/webpack-encore#889

The tl;dr is this:

A) If you're using Encore today, you probably already have this package. You only don't if you have a bit of an older version of @babel/preset-env B) If you're using Encore 1.0, then you will DEFINITELY have it because of symfony/webpack-encore#889

We don't have a release of stimulus-bridge yet that includes the @babel/plugin-proposal-class-properties. My vote is to remove it before we ever have a release. And if people get an error that the package is missing (because they are in the edge-case situation), then the fix is for them to install the package or upgrade @babel/preset-env. Either way, the dependency should not be here.

Thanks!

tgalopin commented 3 years ago

Thanks Ryan.