ember-svg-jar icon indicating copy to clipboard operation
ember-svg-jar copied to clipboard

just assetResolve interface

Open betocantu93 opened this issue 3 years ago • 9 comments

@jherdman here's only the interface PR we talked about at #189

A few things to notice:

  1. It guards against fastboot, not sure how dynamic imports work in fastboot (guidance welcome)
  2. If we are going to write tests, we will need to just don't run them for embroider, at least for hbs strategy at this moment, not sure how to have some sort of "allowlist". For them to work for embroider builds, a whole new strategy with webpack loaders probably should be used instead, I'm working on it... from time to time.

betocantu93 avatar Mar 31 '21 17:03 betocantu93

Yes, I just need to discover how to avoid running those tests for embroider @jherdman

betocantu93 avatar Mar 31 '21 19:03 betocantu93

I've been thinking and maybe dropping embroider tests is the best way at least for now? since it would be unexpected for embroider builds to break if they use this strategy?

betocantu93 avatar Apr 05 '21 16:04 betocantu93

I hate to do this, but I think we need to temporarily revert this strategy. I definitely merged it prematurely, and I'm not comfortable leaving the master branch in a state that's not deployable. I sincerely apologize, this is entirely my fault. Would you be amenable to working with me on a new pull request that introduces this behaviour when it's ready?

Yes I understand, again the only problem at the moment is that hbs strategy is simply not compatible for embroider builds and I don't think it ever will as a v1 addon, as suggested by ef4 here. A whole new addon/strategy/refactor should be used instead: a v2 addon is the right way. A major version with probably breaking changes is inevitable... eventually, so there's no point in testing for embroider IMHO if we want an hbs strategy to work in the meantime.

betocantu93 avatar May 20 '21 17:05 betocantu93

@betocantu93 what do you think the path forward is here?

MelSumner avatar Sep 16 '21 15:09 MelSumner

@betocantu93 what do you think the path forward is here?

Hello @MelSumner we are using this strategy on production without issues with classic builds, here's the example: https://github.com/prysmex/ember-eui/blob/master/site/ember-cli-build.js

I also added an option to include them in the addon tree, instead of public, if that's desired, so the components could be bundled, this was needed for Cordova apps, we had to bundle them since apps for android or iOS won't dynamically load extraneous JavaScript

I think the correct way to go is a new V2 addon, or even just a custom webpack svg-hbs-loader for embroider

IMHO I would leave ember-svg-jar for classic builds.

The other alternative is having a weird build pipeline which does something different if running on embroider or classic, which would be hard to maintain and superseded by the loader

betocantu93 avatar Sep 16 '21 16:09 betocantu93

@jherdman how does that sound to you?

Also for clarity, @betocantu93 are you suggesting a major version bump that is the v2 of this same addon, or are you suggesting that we create a separate addon?

MelSumner avatar Sep 16 '21 19:09 MelSumner

@MelSumner if a reasonable path forward cannot be determined with the package as-is, I'm all for something new.

jherdman avatar Sep 16 '21 19:09 jherdman

@betocantu93

  • is there already an issue to track this?
  • do you think that we can revamp the current addon to be a v2 addon, or should we start a different package?

MelSumner avatar Sep 17 '21 15:09 MelSumner

@betocantu93

  • is there already an issue to track this?
  • do you think that we can revamp the current addon to be a v2 addon, or should we start a different package?

No, there's no issue yet.

Just my questions around the loader here: https://github.com/embroider-build/embroider/issues/746

Not sure yet, but I think the loader could be a simple webpack plugin and svg jar just focus on the UI/discovery part.

I had a PoC, but had a few issues compiling the templates, I'll try again soon.

betocantu93 avatar Sep 19 '21 16:09 betocantu93