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

Update support matrix #174

Closed

Conversation

NullVoxPopuli
Copy link
Contributor

No description provided.

@jrjohnson
Copy link
Collaborator

My only objection is depending on the unstable getGlobalConfig from @embroider/macros without any guarantees around this API or its compatibility with older builds. It's super difficult to actually test application configuration in the dummy app, so we'll need to validate this change continuously with any version bump of embroider. I'm hesitant to take on that maintenance burden and I'm wondering if embroider-build/embroider#770 means we can keep our existing usage of ember-get-config?

@NullVoxPopuli
Copy link
Contributor Author

Unless embroider-build/embroider#770 isn't released, that PR wasn't enough to not need the changes in this PR (#174): NullVoxPopuli/limber#114

without any guarantees around this API or its compatibility with older build

is ember-try not sufficient? 🤔

@jrjohnson
Copy link
Collaborator

jrjohnson commented Jun 8, 2021

is ember-try not sufficient

I wish! But testing an add-on against multiple different application configurations isn't doable without jumping through hoops.

I believe the compatibility fixes are in 0.41.0 so it is released, sad it isn't working. I'll find some time to poke around in getGlobalConfig and see how complicated it is (and how likely it is to be a problem for us down the road).

@NullVoxPopuli
Copy link
Contributor Author

anything keeping this from being merged? what are remaining tasks?
I've been happily pointing at this fork since I put up the PR :D

@jrjohnson
Copy link
Collaborator

It's on my list for today. Wanna rebase it?

@jrjohnson
Copy link
Collaborator

Most of the changes are now done in other work, so it should just be adding the config macro and activating the tests in GitHub workflows. I'll take care of it if you'd like. Thanks for the work and the help figuring out how this fits together.

@jrjohnson
Copy link
Collaborator

Actually looking at discord chat @NullVoxPopuli I think I'd like to hold off to see if ember-get-config can get working with embroider. That would be my idea upgrade path because it seems less risky.

@NullVoxPopuli
Copy link
Contributor Author

what's the verdict?

@jrjohnson
Copy link
Collaborator

Waiting on mansona/ember-get-config#29 As far as I can tell something needs to be fixed in embroiderer.

@davideferre
Copy link
Contributor

Waiting on mansona/ember-get-config#29 As far as I can tell something needs to be fixed in embroiderer.

@jrjohnson ember-get-config is been updated to 1.0.0 version with Embroider support!

@jrjohnson
Copy link
Collaborator

Thanks @davideferre, if you feel like opening a PR with that update I'd be happy to merge it.

@davideferre
Copy link
Contributor

Thanks @davideferre, if you feel like opening a PR with that update I'd be happy to merge it.

Thanks @jrjohnson I've created PR #192

@jrjohnson
Copy link
Collaborator

Thanks for getting this so far @NullVoxPopuli and for the advocacy upstream to get everything in shape. Closing in favor of #192

@jrjohnson jrjohnson closed this Jan 15, 2022
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.

3 participants