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

Fix accounts-twitter and accounts-facebook to not force Blaze into an app #7715

Closed
queso opened this Issue Aug 26, 2016 · 98 comments

Comments

Projects
None yet
@queso
Contributor

queso commented Aug 26, 2016

This is an open issue to discuss the idea of getting Blaze deps out of packages like accounts-twitter and accounts-facebook so that we don't force Blaze on people who may be using React/Angular/Vue, etc.

This is an extension of a discussion started on the forum and intended to discuss ideas and whatnot around this.

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

So @SachaG suggested that we could just make accounts-ui and Blaze a weak dependency for accounts-twitter, et al. Not sure that will work because accounts-facebook just relies on accounts-oauth and accounts-base (https://github.com/meteor/meteor/blob/devel/packages/accounts-facebook/package.js). Going to dig deeper.

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

So it does look like accounts-base calls in Blaze as a weak dependency (https://github.com/meteor/meteor/blob/devel/packages/accounts-base/package.js#L30), so I have no idea why it is coming along all the way atm. My test project doesn't have anything else needing Blaze. My guess is that some other package is bringing it in that accounts-base relies on.

@SachaG

This comment has been minimized.

Contributor

SachaG commented Aug 26, 2016

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

Nice, I just found that. It looks like the accounts-* oauth packages all rely on a package like the twitter one above. This is forcing Blaze in even thought it is a weak dep for accounts-base.

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

So it looks like the underlying twitter package and it's ilk handle two things and I think those things need to be broken out into two packages...

  1. It handles the actual oAuth calls for each service
  2. It contains the html/js to power the 'configuration' dialog if you are using accounts-ui.

@stubailo: Do you have a preference on how this should be handled to extract these packages apart? My gut says the functionality of point 2 should be moved off to new packages... And the current twitter, facebook, and those types of packages should continue to be required by accounts-twitter.

The second option is to move the code related to point 1 into accounts-twitter directly and make twitter a weak dep that people can install if they want the configuration.

@stubailo

This comment has been minimized.

Contributor

stubailo commented Aug 26, 2016

Hmm, do we think anyone would ever use the Facebook package without accounts-Facebook?

@Primigenus

This comment has been minimized.

Contributor

Primigenus commented Aug 26, 2016

You know what would be great? If I add accounts-facebook and accounts-ui and it just knows which view layer my app is already using and hooks it up for me.

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

You know what would be great? If I add accounts-facebook and accounts-ui and it just knows which view layer my app is already using and composes the right one for me.

I would rather keep this focused on achievable, smaller goals.

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

Hmm, do we think anyone would ever use the Facebook package without accounts-Facebook?

I guess it is possible. Looking at the two packages, it is an odd way to divide the code. The code for dealing with oAuth lives in facebook, but the code for calling the Meteor.loginWithFacebook lives in accounts-facebook. Those two things feel like they should live together perhaps, in my mind.

The existance of the configuration html inside the facebook package is the real problem here. There are plenty of ways to configure the oAuth services without needing the UI.

To answer your original question: maybe? If they do install facebook without accounts-facebook, it would be to get oAuth support without needing Meteor.loginWithFacebook...

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

i don't think making blaze related package a weak dependency would solve this issue because all meteor project still include blaze on server due to boilerplate package.... before we deal with that issue,, weak dependency is not a solution i guess... see my comment on this issue... #6844 (comment)

I disagree, having a weak dependency would solve it, I could care less about the server, I am talking about having Blaze shoved into my client bundle, it affects page load times due to files sizes and bandwidth use.

account-* use blaze for ui and give ability to fill configuration (appId/appSecret) on web page...

the most easiest way to remove blaze dependency is to create new packages... something like... account--without-ui... these package basically account- package without configuration and ui... this way,,, account-*-without-ui will agnostic to and won't depend to any ui...

then,,
install account-* if developer want account with blaze ui and ability to fill configuration on web page (appId/appSecret)..
or,,
install account-*-without-ui if developer want account with no ui dependency and manual config (appId/appSecret)..

See, that actually wouldn't work. We are dealing with two layers of issues here. There is accounts-facebook and facebook packages that are needed to power oAuth. It is the facebook package that has the UI pieces in it purely for configuration ui with accounts-ui. I feel like the oAuth specific code should be moved out of facebook and into accounts-facebook, for example.

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

For completeness sake, I am talking about this code: https://github.com/meteor/meteor/blob/devel/packages/facebook/facebook_client.js

and this code:
https://github.com/meteor/meteor/blob/devel/packages/facebook/facebook_server.js

When talking about oAuth stuff that lives in the facebook package instead of accounts-facebook.

@queso

This comment has been minimized.

Contributor

queso commented Aug 26, 2016

i tested account-*-without-ui on local packages folder and working properly...

@MeKarina did you check your list of js files that are getting push or your .meteor/versions file, I bet you are getting blaze still...

@stubailo

This comment has been minimized.

Contributor

stubailo commented Aug 26, 2016

I think it would be fine to just move everything into the accounts-facebook package, and then have facebook be just the UI part. For backcompat, let's make sure facebook still re-exports all of the stuff it used to, and maybe warns you if you don't have accounts-facebook installed?

I think that will cover all of the bases for someone who was using facebook independently, at least they will be notified they need to change their code.

@stubailo

This comment has been minimized.

Contributor

stubailo commented Aug 26, 2016

@MeKarina want to remove the boilerplate-generator dependency? I feel like you could replace it with handlebars from NPM or something - it's literally just rendering a template to a string.

@laosb

This comment has been minimized.

Collaborator

laosb commented Aug 26, 2016

I think having only ui things in facebook can be confusing. We should directly deprecate that package (but exports what it used to export) and print a message when someone use it. Configuration ui should be in a new package called blaze-accounts-config-*(or something like that).

@stubailo

This comment has been minimized.

Contributor

stubailo commented Aug 26, 2016

That works for me as well - probably isn't much more work to do it that way, and is definitely more correct.

@stubailo

This comment has been minimized.

Contributor

stubailo commented Aug 26, 2016

@MeKarina why not?

@stubailo

This comment has been minimized.

Contributor

stubailo commented Aug 26, 2016

You can easily make static-html work without Blaze, but I think that should be a separate discussion.

@queso

This comment has been minimized.

Contributor

queso commented Aug 27, 2016

I agree with @laosb here, likely better to make a new package with a solid name and deprecate the old one.

@laosb

This comment has been minimized.

Collaborator

laosb commented Aug 27, 2016

So that sounds like:

  1. Move everything except configuration UI from package {provider} to accounts-{provider}. Now accounts-{provider} will also export things like Facebook.
  2. Move configuration UI to blaze-accounts-config-ui-*, and accounts-* can have a weak dependency on it.
  3. Deprecate {provider}. {provider} just imply accounts-{provider}, and will output a message if anyone still use them.
@queso

This comment has been minimized.

Contributor

queso commented Aug 27, 2016

@laosb: that sounds spot on from what I am getting from the conversation. Sound good to you too @stubailo?

@stubailo

This comment has been minimized.

Contributor

stubailo commented Aug 29, 2016

Sounds perfect, let's do it!

@laosb

This comment has been minimized.

Collaborator

laosb commented Aug 29, 2016

Working on it! Let's start with facebook!

@laosb

This comment has been minimized.

Collaborator

laosb commented Aug 29, 2016

@stubailo Updated the solution above to be more detailed. How about it?

@stubailo

This comment has been minimized.

Contributor

stubailo commented Aug 29, 2016

Still sounds good!

@abernix

This comment has been minimized.

Member

abernix commented Jan 31, 2017

Thank you both, @laosb & @hwillson. Also, I just realized accounts-meteor-developer is up for grabs still! I'll do this myself if it comes down to the wire.

Also, can anyone verify my thinking on the upgrader (that is to say, the automated process of upgrading to this new system) here? In looking at my previous comment, I think it's actually a bit more complex. We want to ensure that there is zero broken experience for someone upgrading to Meteor 1.4.3 from previous Meteor versions.

I believe the following transformations should be made on existing apps, where <service> is facebook, google, twitter, github, weibo, meetup or meteor-developer:

  • Existence of accounts-<service> adds <service>-config-ui
  • Existence of <service> changes to <service>-oauth

And I think the rest stays the same?

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 1, 2017

I'll tackle accounts-meteor-developer - I should have it ready by tomorrow morning. Oh, what about accounts-meetup? I think we'll need to tackle that one as well. I can't commit to accounts-meetup before end of day tomorrow, but I'll post back if it looks like I'll have time.

Regarding your plan on the upgrader - yes, makes complete sense to me. You transformations look right - maybe just add meetup to the list (assuming it's done in time).

@laosb

This comment has been minimized.

Collaborator

laosb commented Feb 1, 2017

Do we still maintain accounts-meetup? If we don't, I guess deprecating it like deprecating bootstrap would be fine.

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 1, 2017

accounts-meteor-developer should now be done (see #8305). Good question regarding accounts-meetup @laosb; as far as I know it's still maintained, but I'm not sure how much it's used. Deprecating it might be the way to go.

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 2, 2017

@abernix - what do you think about accounts-meetup; should we refactor it or deprecate it? Either way I can work on this today, so let me know which direction you prefer. Thanks!

@abernix

This comment has been minimized.

Member

abernix commented Feb 3, 2017

@hwillson With 931 lifetime installs, it's the lowest of the accounts-* packages and I'd be hard-pressed to say it's worth the investment of time to update it. If it's super easy, I guess you can do it, but I fully support deprecating it. Those want it still could still use the old version – granted it would load Blaze in, but probably a very, very small percentage of people doing that. Deprecation PR gladly accepted in a similar manner to code-prettify (#8265). If there is actually any backlash, we can un-deprecate it and ask those who need it to make the (relatively) simple changes.

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 3, 2017

Makes sense to me @abernix - I'll make the necessary deprecation changes. Thanks!

@abernix

This comment has been minimized.

Member

abernix commented Feb 3, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 6, 2017

@abernix @laosb - I'm starting to re-think the idea of deprecating accounts-meetup. It isn't currently used by many developers, but the Meteor Meetup community is quite active. I've done a bit of digging around GitHub for users of accounts-meetup, and there are definitely up to date / current projects using it. Given that it's not that much work to update it, I think I'll move ahead with updating it instead of deprecating it. It's already being supported under the Meteor core umbrella (and hasn't really logged any issues in quite some time), so we're not really adding a lot of maintenance time by having it continue to be supported (once these OAuth changes are done that is). I'll submit a PR with these changes shortly (unless of course you guys object! 🙂). Thanks!

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 6, 2017

PR #8321 contains the accounts-meetup changes. Let me know if there is anything else outstanding I can help with, to get this issue resolved. Thanks!

abernix added a commit that referenced this issue Feb 10, 2017

Add an upgrader which modifies `accounts-*` packages automatically.
 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: https://github.com/meteor/meteor/pull/7728œ
  * github: #8303
  * google: #8275
  * meetup: #8231
  * meteor-developer: #8305
  * twitter: #8283
  * weibo: #8302

abernix added a commit that referenced this issue Feb 10, 2017

Add an upgrader which modifies `accounts-*` packages automatically.
 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: https://github.com/meteor/meteor/pull/7728œ
  * github: #8303
  * google: #8275
  * meetup: #8231
  * meteor-developer: #8305
  * twitter: #8283
  * weibo: #8302

abernix added a commit that referenced this issue Feb 10, 2017

Add an upgrader which modifies `accounts-*` packages automatically.
 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 that referenced this issue Feb 10, 2017

Upgrade Facebook Graph API to use v2.8
In testing for #7715, I discovered that the v2.2 Graph API endpoint was still in use in the `facebook` package which was due to sunset on 2017-03-25.

See Facebook Graph API Changelog here:
  https://developers.facebook.com/docs/apps/changelog

When a Graph API endpoint is sunset, it (is claimed) to automatically turn over to the next more recent version, in this case v2.3.

v2.3 has a breaking-change over v2.2, notably listed in "Changes from v2.2 to v2.3":

> [Oauth Access Token] Format - The response format of https://www.facebook.com/v2.3/oauth/access_token returned when you exchange a code for an access_token now return valid JSON instead of being URL encoded. The new format of this response is {"access_token": {TOKEN}, "token_type":{TYPE}, "expires_in":{TIME}}. We made this update to be compliant with section 5.1 of RFC 6749.

This change updates both Graph APIs to v2.8 which has LTS until "At least October 2018".

benjamn added a commit that referenced this issue Feb 10, 2017

Upgrade Facebook Graph API to use v2.8
In testing for #7715, I discovered that the v2.2 Graph API endpoint was still in use in the `facebook` package which was due to sunset on 2017-03-25.

See Facebook Graph API Changelog here:
  https://developers.facebook.com/docs/apps/changelog

When a Graph API endpoint is sunset, it (is claimed) to automatically turn over to the next more recent version, in this case v2.3.

v2.3 has a breaking-change over v2.2, notably listed in "Changes from v2.2 to v2.3":

> [Oauth Access Token] Format - The response format of https://www.facebook.com/v2.3/oauth/access_token returned when you exchange a code for an access_token now return valid JSON instead of being URL encoded. The new format of this response is {"access_token": {TOKEN}, "token_type":{TYPE}, "expires_in":{TIME}}. We made this update to be compliant with section 5.1 of RFC 6749.

This change updates both Graph APIs to v2.8 which has LTS until "At least October 2018".

abernix added a commit to abernix/meteor that referenced this issue Feb 10, 2017

Add an upgrader which modifies `accounts-*` packages automatically.
 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 issue Feb 10, 2017

Upgrade Facebook Graph API to use v2.8
In testing for #7715, I discovered that the v2.2 Graph API endpoint was still in use in the `facebook` package which was due to sunset on 2017-03-25.

See Facebook Graph API Changelog here:
  https://developers.facebook.com/docs/apps/changelog

When a Graph API endpoint is sunset, it (is claimed) to automatically turn over to the next more recent version, in this case v2.3.

v2.3 has a breaking-change over v2.2, notably listed in "Changes from v2.2 to v2.3":

> [Oauth Access Token] Format - The response format of https://www.facebook.com/v2.3/oauth/access_token returned when you exchange a code for an access_token now return valid JSON instead of being URL encoded. The new format of this response is {"access_token": {TOKEN}, "token_type":{TYPE}, "expires_in":{TIME}}. We made this update to be compliant with section 5.1 of RFC 6749.

This change updates both Graph APIs to v2.8 which has LTS until "At least October 2018".

abernix added a commit that referenced this issue Feb 10, 2017

Add an upgrader which modifies `accounts-*` packages automatically.
 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
@stubailo

This comment has been minimized.

Contributor

stubailo commented Feb 22, 2017

@abernix is this closed now?

@abernix

This comment has been minimized.

Member

abernix commented Feb 22, 2017

Yes! This splitting of accounts-* packages has been released in Meteor 1.4.3.1. While not officially recommended yet (In other words: developers receiving "update" notifications. Coming soon!), you can test this now by upgrading to Meteor 1.4.3.1:

meteor update --release 1.4.3.1

1.4.3.1 has an automatic upgrader in place which should split packages in .meteor/packages automatically. Currently, it errs on the side of caution and tries to keep the app in the exact same state it was but if Blaze is not needed developers can remove the {service}-config-ui package from their apps with meteor remove <package>.

Just as a reminder, Blaze might still exist on the server-side (for boilerplate HTML reasons), however if you do not actually use Blaze as your view layer on the client, it will no longer be included in the client bundle. This should be another milestone for reducing client bundle sizes for those using other view layers (React, Angular, etc.).

I'd like to give a huge shoutout to @laosb and @hwillson for submitting the (many!) PRs that made this all happen, and a big thanks to everyone else who participated in the design discussion as well.

@abernix abernix closed this Feb 22, 2017

abernix added a commit that referenced this issue Mar 28, 2017

[backport] Upgrade Facebook Graph API to use v2.8
This is a backport of 873f13d for the
Meteor 1.4.2.x series.

In testing for #7715, I discovered that the v2.2 Graph API endpoint was still in use in the `facebook` package which was due to sunset on 2017-03-25.

See Facebook Graph API Changelog here:
  https://developers.facebook.com/docs/apps/changelog

When a Graph API endpoint is sunset, it (is claimed) to automatically turn over to the next more recent version, in this case v2.3.

v2.3 has a breaking-change over v2.2, notably listed in "Changes from v2.2 to v2.3":

> [Oauth Access Token] Format - The response format of https://www.facebook.com/v2.3/oauth/access_token returned when you exchange a code for an access_token now return valid JSON instead of being URL encoded. The new format of this response is {"access_token": {TOKEN}, "token_type":{TYPE}, "expires_in":{TIME}}. We made this update to be compliant with section 5.1 of RFC 6749.

This change updates both Graph APIs to v2.8 which has LTS until "At least October 2018".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment