imgix / ember-cli-imgix

Easily add imgix functionality to your Ember application
https://imgix.github.io/ember-cli-imgix
MIT License
26 stars 13 forks source link

Support for usage with Embroider apps #210

Closed rahulk94 closed 2 years ago

rahulk94 commented 2 years ago

Question Embroider is the latest build tooling for Ember applications which requires some addons to be updated to integrate with it correctly.

The Embroider team have requested that addon authors add Embroider support cases to their Ember-try configuration to find integration issues.

Is this something this project is planning on doing?

Fwiw my team is currently using ember-cli-imgix and are trying to migrate to Embroider however have run into issues with this package's usage of babel-plugin-inline-package-json. There is a Embroider issue raised as well but it is probably good to get official word on whether ember-cli-imgix is going to be updated to support Embroider + having compatibility testing here is probably wise.

rahulk94 commented 2 years ago

Fwiw I've raised a PR to add Embroider to the Ember Try compatibility scenarios here if we're keen to support Embroider https://github.com/imgix/ember-cli-imgix/pull/211

ef4 commented 2 years ago

This addon is running its test suite using ember-cli 3.1. Embroider only tests for support back to ember-cli 3.16. So the first thing to do is to use something like ember-cli-update to get the dummy app updated.

Beyond that I think you'll only need to replace this:

https://github.com/imgix/ember-cli-imgix/blob/ff524c2488b53e9ff146b084c0c0d6be1d3944aa/addon/common/constants.js#L2

The problem isn't exactly with relying on babel-plugin-inline-package-json, because @embroider/compat takes care of running your custom babel plugins. The problem is that the relative path from constants.js to package.json is different after the addon has been converted to v2 format.

rahulk94 commented 2 years ago

Thanks for the reply @ef4.

Yup I've forked this and have removed that babel plugin here and gotten past the immediate issue of that failing babel plugin (and now on to the next one 😂 ).

Depending on whether the maintainers here want to support Embroider, I can probably help with updating the addon to use a newer Ember version.

frederickfogerty commented 2 years ago

Hi @rahulk94 and @ef4, Fred from @imgix here! Thanks for opening and discussing this issue. We weren't aware of Embroider so thanks for raising this with us. We still need to evaluate whether we want to support Embroider, so I would like to ask a few questions to you that might help us evaluate this. What is the background of Embroider? Is it built and maintained but the official Ember core team? And what is the likelihood that it will become significantly used by the Ember community?

After we've answered these questions, we can decide if we want to support Embroider. Of course at that point we would welcome any help you would like to give us, as we are currently flat out building and maintaining other imgix integrations.

Looking forward to hearing back from you!

ef4 commented 2 years ago

The short answer is "yes", the longer answer is:

The Addon Author Guide is the best starting point for getting the lay of the land. For ember-cli-imgix in particular, I think compatibility is probably very easy and I would PR it myself if somebody else can help get the test suite updated to at least ember-cli 3.16 first.

frederickfogerty commented 2 years ago

Thanks so much for all that information @ef4. Reading through those RFCs does give me confidence for sure.

We could potentially help get ember-cli-imgix updated to work with 3.16 - do you maybe have an idea of how much effort that could take?

rahulk94 commented 2 years ago

Using the documentation from ember-cli-update it can be quite easy to upgrade an add on. I've done one recently for an internal add on and it was quite painless as long as you are confident in your test coverage. It updates the app blueprint + exposes codemods you can apply against the codebase.

The main thing will be addressing Ember deprecations as they start showing on later versions and/or whether you decide to upgrade any other components at the same time (e.g. the imgix core library has been renamed from what I can tell so it might be good to do this at the same time).

frederickfogerty commented 2 years ago

Thanks for that info @rahulk94! I'll discuss this with my team and update here next week.

frederickfogerty commented 2 years ago

Hey @rahulk94 and @ef4, I'm here with an update (as promised). So, we agree that Embroider is the way forward and we are happy to help put the work in to support it and also upgrade to Ember 3. Unfortunately, we are currently very busy with two large integrations and don't foresee any spare time in the next while. Therefore we cannot commit to a timeline to supporting Embroider at this stage. On the other hand, we always welcome community submissions, and can always make time to review and handle community PRs. Let me know if this interests either of you. Otherwise, thanks for the contribution so far, it's much appreciated!

rahulk94 commented 2 years ago

Thanks for the update @frederickfogerty. Atm I don't have capacity to help upgrade this to a new Ember CLI version as I'm in the middle of trying to spike into an Embroider POC for our application however if that goes well I may be able to contribute. I'll loop back here if I have capacity to do this though.

For awareness of others, looks like there are some issues with configuration lookups in the ImgixImage component when using Fastboot and Embroider. I've yet to workaround this but will share if I can find one.

Edit: Hmmm so I had a compat adapter for ember-get-config set to null due to other issues earlier (per https://github.com/embroider-build/embroider/issues/823) but I've removed it now and the ImgixImage component seems to be happy again. Don't see any immediate regressions so I guess the observation is that we need that compat adapter to make this work as expected.

Edit 2: Okay so I have ember-get-config issues again once I enable the staticHelpers Embroider option. Fun. I'll see if I can loop back around to this sooner than later.

rahulk94 commented 2 years ago

I'll raise a PR to update Ember CLI next week but am not sure how to get the package.json.version for the constants file. The current babel-plugin-inline-package-json project seems like it won't work anymore as it seems to only work with Babel@6 and upgrading Ember CLI bumps us to Babel@7. Any alternatives would be very helpful if either of ya'll know of any.

ef4 commented 2 years ago

In index.js:

module.exports = {
  options: {
    '@embroider/macros': {
      setOwnConfig: {
        version: require('./package').version
      },
  }
}

Then in the code you can say:

import { getOwnConfig } from '@embroider/macros';
const APP_VERSION = getOwnConfig().version;

This works always under Embroider. It works in classic builds as long as you add @embroider/macros as a dependency of this addon.

frederickfogerty commented 2 years ago

If it becomes a significant blocker, it's also possible to inline the package version, and we would update the version manually upon release.

rahulk94 commented 2 years ago

@ef4 thanks mate that seems to work 🙂 I think Embroider safe and optimized modes might have issues still but I'll poke around a bit more at that after the Ember CLI is updated first.

rahulk94 commented 2 years ago

Well Ember CLI upgrade is almost done. @ef4 looks like next issue after adding embroider and ember-cli-imgix to an app, there are issues with ember-get-config. I know theres an open issue around this (https://github.com/embroider-build/embroider/issues/823) where you mentioned you could remove the current adapter but it seems things are broken either way 😞 .

I've found you've also got a PR open to fix something with that adapter https://github.com/embroider-build/embroider/pull/1030 but unsure if that'll help here.

The errors tend to range from there being no config (trying to get imgix config off an undefined config object) or webpack module resolution fails. Either of these ring any bells from issues seen on other projects? Any suggestions would be much appreciated.

rahulk94 commented 2 years ago

Embroider@0.48.1 has fixed up the ember-get-config compat adapter so this is looking like its embroider compat now (not sure about optimizing)

rahulk94 commented 2 years ago

Version 3.0.1 onwards should give Embroider compatibility as it now uses ember-get-config@1 which is also Embroider compatible. We should be sweet to close this. There maybe some optimizations that can be done still but think those can be tackled separately to this issue.