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

Documentation: Details/Notes regarding Rails API-only applications #5152

Merged
merged 3 commits into from
Oct 29, 2019

Conversation

colinross
Copy link
Contributor

This is a bit of a knock on PR related to #4947 specifically addressing documentation.

I have been experimenting more and more with rails api-only application leveraging devise this PR attempts to make a solid attempt to stem the documentation issues (or really lack thereof) from creating unnecessary noise as bugs or feature requests.

@sourcelevel-bot
Copy link

Hello, @colinross! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@colinross
Copy link
Contributor Author

I'm not quite sure how to best add this point to the documentation, but there is a slight, nuanced difference between an Rails API-only Application and an application where your resources (the protected controllers for instance) are extending from ActionController::API. In comes down to which controller type your devise parent controller (ApplicationController by default) extends from.

While outside the scope of this documentation PR, I wonder if the better way to 'address supporting' API controllers would be to issue warnings when known incompatibilities are loaded/used. e.g.
Rails.logger "Rememberable is not compatible with controllers extending from ActionController::API; Either disable/remove the :rememberable module in the model's configuration or extend from ActionController::Base"

Copy link
Contributor

@mracos mracos left a comment

Choose a reason for hiding this comment

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

Nice job @colinross! 💙
I left a couple of comments, let me know wdyt about them. 😄

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/generators/templates/README Outdated Show resolved Hide resolved
Co-Authored-By: Marcos Ferreira <mracos@users.noreply.github.com>
@colinross
Copy link
Contributor Author

Those edits are fine. Thanks for the time.

Copy link
Contributor

@mracos mracos left a comment

Choose a reason for hiding this comment

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

Just a couple of nits them we are good to go!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-Authored-By: Marcos Ferreira <mracos@users.noreply.github.com>
@colinross
Copy link
Contributor Author

Ahh, good catch

@mracos mracos merged commit 14863ba into heartcombo:master Oct 29, 2019
@mracos
Copy link
Contributor

mracos commented Oct 29, 2019

Thanks for your contribution @colinross. 💙

About incompatibilities between some features of devise and ActionController::API I think a good start is to map some of them (you already mentioned rememberable).

With that info, we can then decide the best course of action for each one, which could be to raise a warning as you said or even just documenting it somewhere. The point is that we would need to have this somehow mapped.

Makes sense to you?

@colinross
Copy link
Contributor Author

That makes sense to me. I'll see what I can get started on and get a PR in the near future. I'm seeing maybe a table of 'Feature/Module' with Supported Y/N and a place to detail the caveats? I'm pretty sure Omniauth will have a caveat or two.

Maybe a FEATURE_SUPPORT.md file? Not sure if i want to muddy up the already quite extensive README.md

@mracos
Copy link
Contributor

mracos commented Oct 29, 2019

WDYT about starting with a wiki article and then later we decide where to put it?

@colinross
Copy link
Contributor Author

colinross commented Oct 29, 2019 via email

@colinross
Copy link
Contributor Author

Started a wiki entry

https://github.com/plataformatec/devise/wiki/API-Mode-Compatibility-Guide

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

Successfully merging this pull request may close these issues.

3 participants