Meteor: Fix accounts-twitter and accounts-facebook to not force Blaze into an app

Created on 26 Aug 2016  Â·  98Comments  Â·  Source: meteor/meteor

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.

few Accounts (in user apps) has-workaround Feature

Most helpful comment

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.

All 98 comments

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.

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.

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.

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.

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

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.

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.

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...

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.

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.

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...

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.

@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.

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).

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

@MeKarina why not?

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

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

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.

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

Sounds perfect, let's do it!

Working on it! Let's start with facebook!

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

Still sounds good!

Do we need to keep a copy of configuration UI templates inside facebook to keep the compatibility?

I think facebook can just have a dependency on the UI package?

Oh, yeah.

@stubailo I submitted #7728 for this.

Apologies for coming late to the party here folks but although I agree with the general thrust of this, I think we should do it slightly differently.

  1. I think we should keep the facebook package. I'm not sure if people use it much (for doing facebook oauth without using the accounts system), I don't see a compelling reason to break backcompat here.
  2. I'd prefer a solution where if you are using accounts-ui and you add accounts-facebook you get the UI configuration without an extra step (which would break a _whole_ lot of existing tutorials etc).

What do we think about just adding the facebook config template (and eventually the other core accounts-X UI templates) to accounts-ui? I know it's bloat but that package is rarely used in production and templates are pretty tiny.

It also means treating the "core" oauth packages differently, but I'm OK with that.

Removing pull-requests-encouraged until we sort out the exact implementation.

@laosb said (on the PR):

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.

It would probably be possible to add something like:

// in package.js
api.use(['accounts-ui', 'accounts-facebook-ui', {weak: true});

// in a js file
if (Package['accounts-ui'] && !Package['accounts-facebook-ui']) {
  // log a warning
}

I think I'd be OK with this. What do people think?

I know it's bloat but that package is rarely used in production and templates are pretty tiny.

So you would remove separation between packages and now accounts-ui would contain all configs for other packages? Isn't this ugly? Or am I misunderstanding something.

So I agree that settings template for Twitter and Facebook are used in production rarely as it is, but I am not sure if accounts-ui is used rarely.

So we don't deprecate {provider} and just move configuration ui templates to a separated package? Actually I didn't see the sense of a package only do the half of the whole thing and actually we do make facebook exports what it used to export. It is back compatible.

@laosb the idea is that people that aren't using the accounts system may still want to do oauth with FB, and the facebook package helps with that. I would say it's a rare use case, and if we were starting from scratch, I wouldn't bother with it, but it seems like there's no reason to deny this use case at this point.

So I agree that settings template for Twitter and Facebook are used in production rarely as it is, but I am not sure if accounts-ui is used rarely.

OK, I think I'm convinced.

So my proposed design now would be
a) Create a new accounts-ui-facebook-config (I think this is a more appropriate name, given accounts-ui is blaze always) that takes the config stuff from facebook

b) Log a warning if you are using accounts-ui and accounts-facebook but not accounts-ui-facebook-config (ala https://github.com/meteor/meteor/issues/7715#issuecomment-243606006)

Why we do not create two packages accounts-ui-facebook-config and facebook-core, and then make facebook depend on accounts-ui-facebook-config and facebook-core?

@mitar I don't think there's a use-case that makes sense for using facebook + accounts-ui but not accounts-facebook. Am I missing something?

I do not think so, it was just to make it backwards compatible. But you are right, warning is probably also good.

(Or exception?)

I think there is a legitimate use case for wanting the accounts UI and facebook oauth, but not the facebook configuration screen, so I'd say a warning

i add more explanation to my original proposal... =))

current structure...

accounts-{service}
=> accounts-base
=> underscore
=> random
=> accounts-oauth
=> {service}
- {service}.js

{service}
=> oauth2
=> oauth
=> http
=> underscore
=> service-configuration
=> random
=> templating
- {service}_server.js
- {service}_client.js
- {service}_configure.html
- {service}_configure.js

proposed structure....

accounts-{service}
=> accounts-base
=> underscore
=> random
=> accounts-oauth
=> {service}
- {service}.js

accounts-{service}-no-ui
=> accounts-base
=> underscore
=> random
=> accounts-oauth
=> {service}-oauth
- {service}.js

{service}
=> {service}-oauth
=> {service}-config

{service}-oauth
=> oauth2
=> oauth
=> http
=> underscore
=> random
=> service-configuration
- {service}_server.js
- {service}_client.js

{service}-config
=> templating
- {service}_configure.html
- {service}_configure.js

usage...

  • blaze..
    meteor add accounts-{service}
  • non blaze...
    meteor add accounts-{service}-no-ui

bonus package... =))
meteor add {service}-oauth
if someone need {service} oauth with manual configuration without ui for adding clientId and of course without adding blaze package...

pros

  • no breaking change for meteor add accounts-{service}.. i guess...
  • no breaking change for meteor add {service}.. maybe...
  • safer from unexpected behavior or unexpected usage.. maybe..

cons

  • more complex... or maybe no because i just simply break {service} package into {service}-oauth and {service}-config...

Maybe facebook-oauth is a better name for a package than just facebook. That's why I suggest deprecating facebook.

I think @MeKarina might be onto something, although I think it's a little too complicated - I don't think we need the flexibility to install {service}-config without accounts-{service}

So what about this proposal:

  • accounts-{service} = accounts-{service}-no-ui + the configuration UI
  • accounts-{service}-no-ui = {service} + accounts integration
  • {service} = just the oauth bit (we could rename this to {service}-oauth potentially).

What I like about this:

  • We keep back-compat completely (except that {service} loses the useless config UI), so all existing documentation / demos work

What I don't like:

  • accounts-{service}-no-ui is kind of wordy
  • People will end up with the UI if they don't think too hard, which might be a poor default.

@tmeasday

loses the useless config UI

i don't think it is useless... it create wow impression for potential new user... including me when i tried meteor todo demo for the first time... =))

accounts-{service}-no-ui is kind of wordy

agree,, =P

this a bit better maybe...

accounts-{service}
=> accounts-{service}-no-ui
=> {service}-config

accounts-{service}-no-ui
=> accounts-base
=> underscore
=> random
=> accounts-oauth
=> {service}-oauth
- {service}.js

{service}
=> {service}-oauth
=> {service}-config

{service}-oauth
=> oauth2
=> oauth
=> http
=> underscore
=> random
=> service-configuration
- {service}_server.js
- {service}_client.js

{service}-config
=> templating
- {service}_configure.html
- {service}_configure.js

usage is the same...

i feel we should keep {service} package since it just combining {service}-oauth and {service}-config...

@MeKarina

i don't think it is useless... it create wow impression for potential new user... including me when i tried meteor todo demo for the first time... =))

Oh, if it was unclear, I meant that it's useless if you aren't using accounts (i.e facebook but not accounts-facebook).

BTW, looking into this I realized that anyone can configure services even in production if they were left unconfigured. It would be great if we could remove that feature as well: https://github.com/meteor/meteor/issues/7745 So that if you want this simple configuration + configuration template you should add some package. But otherwise service-configuration package should just give you a server side collection.

Could it be possible for developer to left their login unconfigured in production..?? Signup/login is the very first gate no..?? It might be the very first they would tested before going to production..??

On 1 Sep 2016, at 13:22, Mitar [email protected] wrote:

BTW, looking into this I realized that anyone can configure services even in production if they were left unconfigured. It would be great if we could remove that feature as well: #7745 So that if you want this simple configuration + configuration template you should add some package. But otherwise service-configuration package should just give you a server side collection.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

I think when somebody is just deploying an app this could happen. Just imagine app like Rockat.Chat. (An abstract case, I am not sure if they really do it like that, but it is an example of an app in Meteor.) You deploy a Docker container. And you do not configure Facebook integration because you do not want it. And they do not use accounts-ui for UI, so there is no configuration prompt displayed. But their method is exposed so somebody can still configure it by calling the method directly the first time.

So, apps which load packages, so that potential deployers of the app can configure integrations, if they want. But some might not want it, so they are left open.

Basically yes, many people are doing like this, which is a (maybe) potential security issue. (Please keep further discussions in #7745, since it's not the same issue.)

IMO the proposal by @MeKarina is still too complex. Also, it shouldn't be {service}-config which is also misleading, since they're templates for accounts-facebook.

Now seems whether the configuration templates should be default in accounts-{service}. I personally like a view-layer-free accounts-{service}, since it's easier to understand and clear even if you just look at the package names to guess what to do. The name, accounts-{service}, doesn't suggest some templates in it. We'll just need to update meteor add accounts-facebook to meteor add accounts-facebook accounts-facebook-config-ui to make a wow experience for beginners. (They even partly know how does the _Meteor magic_ work since it's clear that "to get the config ui you need some templates for it" by having this command instead of just an accounts-facebook).

The package name of the config UI may need further discussions too, while I suggest accounts-facebook-config-ui which is clear enough, I think.

@mitar ahhh i see i see never imagined such scenario

@wexpolyu
Yepp,,everything has pros and cons...

We could make it back compat for the cost of making system a little too complex... We could make it simpler for the cost of breaking change and depreciating package or functionality..

On 2 Sep 2016, at 05:16, Wexpo Lyu [email protected] wrote:

IMO the proposal by @MeKarina is still too complex. Also, it shouldn't be {service}-config which is also misleading, since they're templates for accounts-facebook.

Now seems whether the configuration templates should be default in accounts-{service}. I personally like a view-layer-free accounts-{service}, since it's easier to understand and clear even if you just look at the package names to guess what to do. The name, accounts-{service}, doesn't suggest some templates in it. We'll just need to update meteor add accounts-facebook to meteor add accounts-facebook accounts-facebook-config-uito make a wow experience for beginners. (They even partly know how does the _Meteor magic_ work since it's clear that "to get the config ui you need some templates for it" by having this command instead of just anaccounts-facebook`).

The package name of the config UI may need further discussions too, while I suggest accounts-facebook-config-ui which is clear enough, I think.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@MeKarina: No worries. I also realized it only once I started replying to your question. Keep up with good work, proposals, and questions.

Deprecating facebook shouldn't have backcompat issues, since facebook will still do what it used to do but with a deprecation notice which leads users to use facebook-oauth instead.

Ok, so how does everyone feel about this solution:

  1. Config UI removed from {service} -> {service}-config-ui
  2. Remainder of {service} -> {service}-oauth, with old package implying it w/ deprecation warning
  3. accounts-{service} depends only on {service}-oauth, but does log the warning above if the user is also using accounts-ui but not {service}-config-ui

(I think this probably a proposal made by @laosb before :) )

Almost, I guess.

Can people let us know if they have objections to this proposal? Otherwise I'm going to mark it PRs-encouraged.

Maybe we should have another label called suggestions-encouraged or something like that?

I think at some point someone has to just make a decision. I guess that's my job :/

So I should update my existing PR and reopen it, or just open a new one? I will work on it tonight(10hrs later), since I'll have a day off from school this afternoon.

I reopened #7728 and updated to your last proposal @tmeasday

The PR is now ready to merge if anyone would like to test it out a bit.

7728 is merged!

Reopening because only Facebook has been split. Twitter (and Google, #4045) should probably be split as well.

Good point, thanks @mitar

But I would suggest that they are done in exactly the same manner. :-)

I assume PRs are welcome for that, just following the other PR as a
template?

On Wed, Nov 9, 2016 at 5:26 AM Mitar [email protected] wrote:

But I would suggest that they are done in exactly the same manner. :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/meteor/meteor/issues/7715#issuecomment-259381348, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAAagb67FgLZDwFgL49ICZm20fr2sz1ks5q8Z_NgaJpZM4JtrBM
.

Yep! Although potentially we should wait until the FB changes have made it into a release to see if there are any big problems people run into.

Thanks very much for this work - I've been trying to track down the blaze/template dependancies for the client, and you guys are already on it. THANKS!


I, personally, am waiting on facebook, google, and twitter and those are the only things putting blaze into my (react UI) app. c'mon and save me many KB!

Any movement on this?

The facebook one should be released in 1.4.3 while others are not separated. Personally I'm having no time working on it.

@zeroasterisk @Method-X I see you're both actively interested in the other providers being ported over (in addition to Facebook). Would either of you be interested in making your first code contributions to Meteor to make this happen? It's mainly a matter of following the pattern of #7728 (Facebook) and therefore a great candidate for a first contribution that's not too complex.

With accounts-twitter and accounts-google completed in the pipeline now, it might be worth considering if this change should implement an upgrader which automatically makes changes to your .meteor/packages, adding new and removing deprecated packages as necessary.

Also, if anyone wanted to tackle the remaining accounts-* packages, accounts-meteor-developer, accounts-github and accounts-weibo, it'd be lovely to get these all into one release! Otherwise, the finished ones are likely to land in 1.4.3.

If no-one else has started it, I'll tackle accounts-github.

accounts-weibo is basically a China-based component. Let me do it if no one else has been working on it.

@laosb and @hwillson Fairly sure they are both up for grabs. If feasible and you can get those done in the next couple days, we can land this entire issue in Meteor 1.4.3 – the only remaining task would be writing an upgrader that switches these packages automatically for the users. This will ensure that google becomes accounts-google-ui AND google-oauth (and the same for the other packages). It would be nice if we could do this in a single upgrader, rather than multiple.

@abernix I've been learning to code since May. If I were more experienced I would help with this in a heartbeat.

@Method-X Realistically, the changes here are pretty straight forward and should (almost?) exactly replicate these changes from the google change. You'd need to analyze that PR's changes and be familiar with running Meteor from checkout. That might be a task in and of itself for you to practice though! That being said, @laosb and @hwillson have already volunteered here so I don't want to double up on efforts until they've opted out. I'd ask that we consider the previously mentioned timeline (by end of Wednesday, perhaps) too though. :)

If anyone is able to test and isn't using accounts-github or accounts-weibo, I think this is ready to test in Meteor 1.4.3-beta.3:

meteor update --release 1.4.3-beta.3

Input would be appreciated!

I'll have accounts-github done first thing tomorrow morning - thanks!

Let me do it now!

accounts-github changes are ready (PR #8303) and verified working with both Blaze and React. Thanks!

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?

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).

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

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.

@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!

@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.

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

Thank you, @hwillson!

@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!

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 is this closed now?

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.

Was this page helpful?
0 / 5 - 0 ratings