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

Support for usage with Embroider apps #210

Closed
rahulk94 opened this issue Oct 27, 2021 · 17 comments
Closed

Support for usage with Embroider apps #210

rahulk94 opened this issue Oct 27, 2021 · 17 comments

Comments

@rahulk94
Copy link
Contributor

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
Copy link
Contributor Author

Fwiw I've raised a PR to add Embroider to the Ember Try compatibility scenarios here if we're keen to support Embroider #211

@ef4
Copy link

ef4 commented Oct 28, 2021

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:

const APP_VERSION = require('../../package.json').version;

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
Copy link
Contributor Author

rahulk94 commented Oct 28, 2021

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
Copy link
Contributor

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
Copy link

ef4 commented Nov 2, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

rahulk94 commented Nov 5, 2021

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
Copy link
Contributor

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

@frederickfogerty
Copy link
Contributor

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
Copy link
Contributor Author

rahulk94 commented Nov 22, 2021

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 embroider-build/embroider#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
Copy link
Contributor Author

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
Copy link

ef4 commented Nov 26, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

rahulk94 commented Dec 1, 2021

@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
Copy link
Contributor Author

rahulk94 commented Dec 3, 2021

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 (embroider-build/embroider#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 embroider-build/embroider#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
Copy link
Contributor Author

rahulk94 commented Dec 9, 2021

[email protected] has fixed up the ember-get-config compat adapter so this is looking like its embroider compat now (not sure about optimizing)

@rahulk94
Copy link
Contributor Author

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.

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

No branches or pull requests

3 participants