Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

betocantu93
Copy link
Contributor

@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.

Copy link
Collaborator

@jherdman jherdman left a 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.

@betocantu93
Copy link
Contributor Author

betocantu93 commented Mar 31, 2021

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

@betocantu93
Copy link
Contributor Author

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?

Copy link
Collaborator

@jherdman jherdman left a 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?

@betocantu93
Copy link
Contributor Author

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.

@MelSumner
Copy link

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

@betocantu93
Copy link
Contributor Author

@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

@MelSumner
Copy link

@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?

@jherdman
Copy link
Collaborator

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

@MelSumner
Copy link

@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?

@betocantu93
Copy link
Contributor Author

@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:
embroider-build/embroider#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants