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

Check host app, then parent, then app for options. #217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GCheung55
Copy link

In an in-repo-engine, options are skipped due to the order of how the options are checked.
Reverse them so host app options are checked first, then parent options, then app options.

Should address #182 (comment)

In an in-repo-engine, options are skipped due to the order of how the options are checked.
Reverse them so host app options are checked first, then parent options, then app options.
@GCheung55
Copy link
Author

GCheung55 commented Jun 4, 2020

@rwjblue any guidance to get this PR approved/merged?

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it seems better in the abstract, to check hostApp before app for sure (that is the goal of this PR), but it seems particularly odd to check either of those before parent.options.

What do you think?

@GCheung55
Copy link
Author

GCheung55 commented Jun 10, 2020

Could you help me understand what parent, app, and hostApp would refer to?

From the docs:

parent

This addon's parent.

If the addon is a direct dependency of an application, then parent will be the corresponding project instance. If it's a dependency of another addon, then parent will be a reference to that addon.

app

The host app instance.

Note: this property will only be present on addons that are a direct dependency of the application itself, not of other addons. It is also not available in init(), but will be set before setupPreprocessorRegistry() and included() are invoked.

hostApp comes from tihs._findHost method

This method climbs up the hierarchy of addons up to the host application.

I'm unsure, but if the addon is in an in-repo-engine then I assume that:

  • parent would refer to the in-repo-engine.
  • app would refer to the host app, so the app that contains (holds?) the in-repo-engine.
  • hostApp would refer to the app, but actually refers to the in-repo-engine.

I'll put together a repo I can share to produce/test this.

@GCheung55
Copy link
Author

@rwjblue Here's the repo to reproduce the issue: https://github.com/GCheung55/ember-sass-test-repo.

Side note, I had to install node-sass instead of using sass (Dart version) because some imports weren't working.

@GCheung55
Copy link
Author

@rwjblue just following up on this in case you've missed it.

@GCheung55
Copy link
Author

@rwjblue Ping - just following up on this since I'm going through an upgrading round again that will likely be blocked by this issue.

@tehhowch
Copy link

tehhowch commented Feb 3, 2023

any updates on this?

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