-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
just assetResolve interface #192
base: master
Are you sure you want to change the base?
just assetResolve interface #192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need some kind of test to ensure it actually works. Ideally example integration with the dummy app. Let me know if you need a hand.
Yes, I just need to discover how to avoid running those tests for embroider @jherdman |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@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: 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 |
@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 if a reasonable path forward cannot be determined with the package as-is, I'm all for something new. |
|
No, there's no issue yet. Just my questions around the loader here: 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. |
@jherdman here's only the interface PR we talked about at #189
A few things to notice:
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.