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

Add assertion to verify preprocessor was run on create-ref modifier #65

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

jasonbekolay
Copy link
Contributor

Our team uses ember-ref-bucket in one of our private addons and an app that uses that addon. We did not have ember-ref-bucket in the dependencies for the app, which caused an issue where we could use create-ref but the bucket was set incorrectly because the preprocessor was not running on the app code.

This took us a while to debug. The assertion in this PR would provide immediate feedback in development/test and the action to take to resolve. I'm happy to change this if you have any feedback.

Thanks for your work on this addon!

@lifeart
Copy link
Owner

lifeart commented Dec 13, 2023

Hi @jasonbekolay, thank you for report!

Technically it's completely allowed to use ref-bucket without preprocessor, but I agree it's barely a real use-case and could lead into issues as mentioned.

I'm afraid if we land change as-is we could break flow for users if already used with this behaviour. Could we replace assert with warn on non-prod envs?

Co-authored-by: Cyril Fluck <github@fluck.fr>
@jasonbekolay
Copy link
Contributor Author

Updated 👍 I've also changed it not warn if the bucket is specified.

@lifeart lifeart merged commit 95aa1d0 into lifeart:master Dec 13, 2023
7 of 10 checks passed
@lifeart
Copy link
Owner

lifeart commented Dec 13, 2023

@jasonbekolay thank you for your work! v5.0.7 published.

@jasonbekolay
Copy link
Contributor Author

Thanks for the quick merge and release 😄

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

2 participants