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

Accounts-facebook reorganize #7728

Merged
merged 18 commits into from Nov 8, 2016
Merged

Accounts-facebook reorganize #7728

merged 18 commits into from Nov 8, 2016

Conversation

laosb
Copy link
Contributor

@laosb laosb commented Aug 29, 2016

#7715

This PR should fix the facebook one. I'm not familiar with the version number based on releases, so the versioning may break though.

@stubailo
Copy link
Contributor

This looks pretty good to me! Waiting on @tmeasday to review and confirm.

@laosb
Copy link
Contributor Author

laosb commented Aug 29, 2016

Note that version numbers was bumped (minor) but I don't know what to do to keep them works together; it might broken. Guessing this must be released in a Meteor release instead of just publishing these packages?

Also, we may need some docs update, since after this PR, you'll need to add an extra blaze-accounts-ui-config-facebook to get the configuration UI.

@stubailo
Copy link
Contributor

@laosb thinking about that, would it be bad to have accounts-ui include all of the Blaze config packages directly? It feels like they are super small, and it would avoid making the setup experience more complex.

@laosb
Copy link
Contributor Author

laosb commented Aug 30, 2016

@stubailo I guess that won't make someone who only use accounts-password happy. It's small but still too much for someone don't use it. Actually I'm using accounts-ui in production with our own styles.

I would suggest printing a message when accounts-{provider} find accounts-ui to tell users add the small ui package. In fact even they do use accounts-ui and accounts-{provider}, many of them will still configure it through environment variables or something like that, for it's easier in production.

@tmeasday
Copy link
Contributor

@laosb this is great but I think we'll end up doing it slightly differently. Let's take the discussion back to the issue?

@laosb
Copy link
Contributor Author

laosb commented Aug 31, 2016

That's OK

@tmeasday
Copy link
Contributor

tmeasday commented Sep 6, 2016

I'm going to close this, we can always re-open if we want to use this code for the ultimate solution.

@tmeasday tmeasday closed this Sep 6, 2016
@laosb laosb reopened this Sep 10, 2016
@laosb
Copy link
Contributor Author

laosb commented Sep 16, 2016

@tmeasday

Copy link
Contributor

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Thanks for this @laosb but it doesn't currently work, let's iterate a little.

@@ -11,13 +11,11 @@ Package.onUse(function(api) {
api.use('underscore', 'server');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of all these api.uses in this package now it is empty?

api.use('http', ['server']);
api.use('underscore', 'server');
api.use('random', 'client');
api.use('service-configuration', ['client', 'server']);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this package should depend on service-configuration, this should be in the config ui package.

Copy link
Contributor

Choose a reason for hiding this comment

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

@laosb we need to remove this, otherwise this package ends up depending on accounts-base which is not what we want.


Package.onUse(function(api) {
api.use('templating@1.2.13', 'client');
api.use('accounts-facebook@1.1.0');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this package should depend on accounts facebook.

a) it leads to a circular dependency
b) the facebook package didn't previously
c) in theory you could have the config ui w/o accounts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, my fault.

@@ -0,0 +1,5 @@
if (Package['accounts-ui'] && (typeof Package['facebook-config-ui'] === undefined)) {
console.info("You're using accounts-ui and accounts-facebook, but didn't",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this message in my testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't do much test on it yet. Will do this weekend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All done!

@laosb laosb force-pushed the accounts-facebook-reorganize branch from 7329f7d to 447f472 Compare September 24, 2016 09:16
@laosb
Copy link
Contributor Author

laosb commented Sep 24, 2016

Rebased.

@laosb
Copy link
Contributor Author

laosb commented Sep 24, 2016

image

@laosb
Copy link
Contributor Author

laosb commented Sep 24, 2016

@tmeasday I guess it's working now.

Copy link
Contributor

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Looking good, just a few minor tweaks now!

@@ -1,23 +1,13 @@
Package.describe({
summary: "Facebook OAuth flow",
version: "1.2.10-beta.5"
summary: "DEPRECATED - Facebook OAuth flow",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put "use facebook-oauth instead" in here somehow?

if (Package['accounts-ui'] && (Package['facebook-config-ui'] === undefined)) {
console.info("You're using accounts-ui and accounts-facebook, but didn't",
"install the configuration UI for Facebook OAuth. You can do it by",
"adding facebook-config-ui.");
Copy link
Contributor

@tmeasday tmeasday Sep 27, 2016

Choose a reason for hiding this comment

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

Can we make this

console.info("You're using accounts-ui and accounts-facebook, but didn't",
 +        "install the configuration UI for Facebook OAuth.");
console.info("You can install it with `meteor add facebook-config-ui`.");

** Deprecated **
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, can we note what people should use instead please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@abernix
Copy link
Contributor

abernix commented Sep 30, 2016

Rebased to get dd3fe9a, to hopefully fix the test which failed last.

@laosb
Copy link
Contributor Author

laosb commented Oct 2, 2016

@tmeasday Done!

@tmeasday
Copy link
Contributor

tmeasday commented Oct 4, 2016

Looks good! Thank you @laosb, great stuff.

@tmeasday
Copy link
Contributor

tmeasday commented Oct 5, 2016

Ok a few more changes were needed, specifically the Accounts UI if you were in the "in between" use case.

Hopefully this should cover all the other oauth packages if people want to similarly change them in the future.

@tmeasday
Copy link
Contributor

tmeasday commented Oct 5, 2016

Those interested (@queso / @mitar / @MeKarina ) it would be great to get some more people playing around with various combinations of accounts-X packages and checking it all makes sense.

@laosb
Copy link
Contributor Author

laosb commented Oct 6, 2016

Having examinations in school... Will help cover other oauth packages!

@laosb
Copy link
Contributor Author

laosb commented Oct 11, 2016

So are we waiting for more users to try it out? PRs get outdated quickly.

@benjamn benjamn self-assigned this Oct 11, 2016
Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

I went through and tested, I think thoroughly, but I don't know React very well.

Using the simple-todos-react demo with these changes I was able to get it working and definitively not load blaze on the client while still allowing Meteor.loginWithFacebook() to work properly. I don't think there are actually -ui components that I can test the configuration part with, but ServiceConfiguration works.

Blaze simple-todos almost worked (no changes to it, itself), but did require a change below with service-configuration (explained below).

api.use('http', ['server']);
api.use('underscore', 'server');
api.use('random', 'client');
api.use('service-configuration', 'client');
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe service-configuration needs to be ['server',client']here otherwise if you don't haveservice-configurationin your.meteor/packages` it will fail when you try to login after the configuration:

  1. Click Configure Facebook Login

    image

  2. Fill in then Save Configuration

    image

  3. It appears to save, and button changes to login

    image

  4. But when you click it, you get Internal Server Error

    image

I tested with server and it worked properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, forgot to include the error:

W20161013-20:04:59.628(3) (oauth_server.js:398) Error in OAuth Server: ServiceConfiguration is not defined
I20161013-20:04:59.893(3)? Exception while invoking method 'login' ReferenceError: ServiceConfiguration is not defined
I20161013-20:04:59.894(3)?     at getTokenResponse (packages/facebook-oauth/facebook_server.js:49:1)
I20161013-20:04:59.895(3)?     at Object.handleOauthRequest (packages/facebook-oauth/facebook_server.js:28:1)
I20161013-20:04:59.895(3)?     at OAuth._requestHandlers.(anonymous function) (packages/oauth2/oauth2_server.js:8:1)
I20161013-20:04:59.895(3)?     at middleware (packages/oauth/oauth_server.js:173:1)
I20161013-20:04:59.896(3)?     at packages/oauth/oauth_server.js:146:1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fault. I did test the deprecation notice and roughly use a code-configured service to test the ui.

if (Package['accounts-ui'] && (Package['facebook-config-ui'] === undefined)) {
console.info("You're using accounts-ui and accounts-facebook, but didn't",
"install the configuration UI for Facebook OAuth.");
console.info("You can install it with `meteor add facebook-config-ui`.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I'd make these .warn, just like the deprecation warning below. I think it looks nicer.

@@ -0,0 +1,18 @@
Package.describe({
summary: "Facebook OAuth flow",
Copy link
Contributor

Choose a reason for hiding this comment

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

name?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't seem to do this for core packages, so I won't break the rule here.

@@ -0,0 +1,3 @@
# facebook-oauth

An implementation of the Facebook OAuth flow. See the [project page](https://www.meteor.com/accounts) on Meteor Accounts for more details. XXX link
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still XXX link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not very sure about what link is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the current link is fine. Maybe just remove XXX link? 😄

@@ -0,0 +1 @@
.build*
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this put here automatically?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see anything that would create it automatically but upon further inspection, I guess it's desired per 8dbee56 and 8bf2e0e. I still don't quite understand, but it doesn't matter/I was just curious.

@@ -0,0 +1,13 @@
Package.describe({
summary: "Blaze configuration templates for Facebook OAuth.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's not necessary, but do these need name? Downsides? Upsides? 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah defaulting to using the directory name is kind of ughh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just copying the existing package.js. It does work and if we add name, we should add it in other packages too.

@tmeasday
Copy link
Contributor

I'll try to fix these problems up monday

@laosb laosb force-pushed the accounts-facebook-reorganize branch from 94b9778 to a4dad54 Compare November 6, 2016 07:21
@laosb
Copy link
Contributor Author

laosb commented Nov 6, 2016

Solved the conflict @tmeasday

@abernix
Copy link
Contributor

abernix commented Nov 7, 2016

@laosb The answer is probably yes, but they probably won't merge this until work is being done on 1.4.3 instead of 1.4.2.1. This isn't appropriate for 1.4.2.1 (a bug-fix release).

@laosb
Copy link
Contributor Author

laosb commented Nov 7, 2016

Yep, should be in 1.4.3.

@tmeasday tmeasday merged commit ef64f7a into devel Nov 8, 2016
@tmeasday
Copy link
Contributor

tmeasday commented Nov 8, 2016

Finally merged! Thanks all!

@laosb laosb mentioned this pull request Jan 31, 2017
abernix added a commit that referenced this pull request Feb 10, 2017
 The transformations to be made here are:
   * Existence of `accounts-<service>` **adds** `<service>-config-ui`
   * Existence of `<service>` **changes to** `<service>-oauth`
 #7715 (comment)

 Relates to:

  * facebook: #7728
  * github: #8303
  * google: #8275
  * meetup: #8231
  * meteor-developer: #8305
  * twitter: #8283
  * weibo: #8302
abernix added a commit to abernix/meteor that referenced this pull request Feb 10, 2017
 The transformations to be made here are:
   * Existence of `accounts-<service>` **adds** `<service>-config-ui`
   * Existence of `<service>` **changes to** `<service>-oauth`
 meteor#7715 (comment)

 Relates to:

  * facebook: meteor#7728
  * github: meteor#8303
  * google: meteor#8275
  * meetup: meteor#8231
  * meteor-developer: meteor#8305
  * twitter: meteor#8283
  * weibo: meteor#8302
abernix added a commit that referenced this pull request Feb 10, 2017
 The transformations to be made here are:
   * Existence of `accounts-<service>` **adds** `<service>-config-ui`
   * Existence of `<service>` **changes to** `<service>-oauth`
 #7715 (comment)

 Relates to:

  * facebook: #7728
  * github: #8303
  * google: #8275
  * meetup: #8231
  * meteor-developer: #8305
  * twitter: #8283
  * weibo: #8302
@abernix abernix deleted the accounts-facebook-reorganize branch April 17, 2017 04:42
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

5 participants