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

docs: update ember-addon example #15

Merged
merged 3 commits into from
Jun 3, 2022

Conversation

ndekeister-us
Copy link
Contributor

@ndekeister-us ndekeister-us commented Oct 22, 2021

Not really important and very specific to ember-addon example, but if we put validatePeerDependencies check in init method, then we will not be able to generate addon blueprints, the check will fails because peerDependencies will not be found

It is an unwanted behavior as, most of the time, ember-addon blueprints will automatically install peerDependencies. Putting the check in included hook allows blueprints to be run correctly and throws error when building/serving the project if peerDependencies are not found

Current behavior with init method ->
Capture d’écran 2021-10-22 à 17 59 33

@ndekeister-us ndekeister-us marked this pull request as ready for review October 22, 2021 16:08
@hjdivad hjdivad requested a review from rwjblue October 27, 2021 23:37
Copy link
Collaborator

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

Seems fine to me.

@rwjblue do you see any issue with recommending validation occur in included rather than init?

@rwjblue
Copy link
Owner

rwjblue commented Oct 27, 2021

I think using included could be fine, but IMHO it isn't a straight up replacement. There are a number of scenarios where you wouldn't be doing a build, but still want to validate (e.g. running tests with a prebuilt dist, adding your own commands, etc).

Maybe instead of replacing, we could add both examples and indicate the pros/cons of each?

@rwjblue
Copy link
Owner

rwjblue commented Oct 27, 2021

if we put validatePeerDependencies check in init method, then we will not be able to generate addon blueprints, the check will fails because peerDependencies will not be found

I don't fully understand this @ndekeister-us, can you explain further? As you mentioned, most blueprints will automatically add the required peer dependencies for you; at which point I would expect the validation in init to actually work just fine?

@rwjblue
Copy link
Owner

rwjblue commented Oct 27, 2021

Ah, I see. The blueprint hasn't been run yet (in the demo), so it hasn't had a chance to install the required peer dependencies.

@rwjblue
Copy link
Owner

rwjblue commented Oct 27, 2021

I think it's probably possible to know that the blueprint is being ran, and choose not to validate. But I think documenting and using included is just going to be simpler to explain (and easier to get right).

@ndekeister-us
Copy link
Contributor Author

Documented the included hook example (but feel free to edit if it is not clear)

Co-authored-by: Robert Jackson <me@rwjblue.com>
@hjdivad hjdivad merged commit 1688e7d into rwjblue:main Jun 3, 2022
@hjdivad
Copy link
Collaborator

hjdivad commented Jun 3, 2022

Thanks @ndekeister-us!

@hjdivad hjdivad added the documentation Improvements or additions to documentation label Jun 3, 2022
@ndekeister-us ndekeister-us deleted the docs/update_ember-addon_example branch June 3, 2022 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants