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

Added a check to enforce top-level dependency #290. #292

Merged

Conversation

dnalagatla
Copy link
Contributor

This PR adds a check to enforce top-level dependency requirement for ember-fetch addon.

index.js Outdated
@@ -68,6 +68,15 @@ module.exports = {
included: function() {
this._super.included.apply(this, arguments);

let isApp = !this.project.isEmberCLIAddon();

let hasEmberFetch = this.project.addons.map(addon => addon.name).includes('ember-fetch');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good, but there is a more concise API that could be used:

project.findAddonByName('ember-fetch') https://github.com/ember-cli/ember-cli/blob/807f56e2e434f716757de02fd1f2c3251dadaf6f/lib/models/project.js#L634

I noticed this method was listed as private, but it's intent was to be public so I've fixed that:ember-cli/ember-cli#8621

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another approach, would be to do this work in the init() would be via this.project.dependencies['ember-fetch']

This approach would allow us to disable nested ember-fetch instances via isEnabled() { } hook, whereas if isEnabled returns false, we won't hit the included hook.

Copy link
Contributor Author

@dnalagatla dnalagatla May 4, 2019

Choose a reason for hiding this comment

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

This looks good, but there is a more concise API that could be used:

project.findAddonByName('ember-fetch') https://github.com/ember-cli/ember-cli/blob/807f56e2e434f716757de02fd1f2c3251dadaf6f/lib/models/project.js#L634

I noticed this method was listed as private, but it's intent was to be public so I've fixed that:ember-cli/ember-cli#8621

I made the change to use findAddonByName API for checking if addon exists. Also, updated the condition to check if the app has ember-cli-fastboot. If that addon exists and ember-fetch is not included throw the error.

Copy link
Contributor Author

@dnalagatla dnalagatla May 4, 2019

Choose a reason for hiding this comment

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

Another approach, would be to do this work in the init() would be via this.project.dependencies['ember-fetch']

This approach would allow us to disable nested ember-fetch instances via isEnabled() { } hook, whereas if isEnabled returns false, we won't hit the included hook.

I am just wondering, using above approeach and disabling nested fetch can have any side-effects.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@xg-wang
Copy link
Member

xg-wang commented May 3, 2019

@dnalagatla @stefanpenner
Just to clarify the hard requirement is for Fastboot support. In app without Fastboot, the problem of nested ember-fetch (or any addon with a custom vendor roll-up) is the first-write-win conflict.
@dnalagatla Can you also update the Readme, moving top-level addon from Use with Fastboot to upper level so it's no longer fastboot specific?

@dnalagatla
Copy link
Contributor Author

dnalagatla commented May 4, 2019

@xg-wang, I agree. I will update the readme to make this the requirement.

You are correct this issue is Fastboot specific and if apps are using fastboot, the ember-fetch as nested dependency fails to set up the fetch module. Apps not on fastboot, I think there should not be an issue.

@stefanpenner @xg-wang
What do you think about checking if the application includes ember-cli-fastboot as a dependency before checking if ember-fetch is installed as a top-level dependency? This way we can keep the text as it is in readme, as current text makes more sense.

@dnalagatla dnalagatla force-pushed the dnalagatla/enforce-top-level-addon branch from 776616f to 11d9d7e Compare May 4, 2019 15:47
@xg-wang
Copy link
Member

xg-wang commented Aug 16, 2019

@dnalagatla Is this PR ready to merge? This requires a major bump that can go together with #348.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants