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

5 ways to fix the great jQuery debacle (no. 4 may surprise you) #5298

Closed
ErisDS opened this issue May 20, 2015 · 13 comments
Closed

5 ways to fix the great jQuery debacle (no. 4 may surprise you) #5298

ErisDS opened this issue May 20, 2015 · 13 comments

Comments

@ErisDS
Copy link
Member

ErisDS commented May 20, 2015

It's time to get rid of jQuery from Ghost's {{ghost_foot}} helper...

The problem

Currently, Ghost's {{ghost_foot}} helper includes jQuery for everyone. Many moons ago, when Ghost's theme API was first born, this seemed like a good idea as it seemed to be something the majority of themes would benefit from, making it worthwhile to add to core.

Overtime, we've come to realise this wasn't a good decision. Themes that don't want or need it have no good way to remove it. Although it is possible to remove using an app, apps aren't officially 'ready' and regardless, forcing people to install an app to disable something is counter intuitive. The result of all this has been many themes not including the required {{ghost_foot}} helper, which means Code Injection doesn't work for everyone, and that's just the start of the problems.

Ideally, jQuery should have been opt-in, rather than opt-out. It's time to fix that.

The solutions

  1. Just remove it
    Unfortunately, many many themes depend on it, so just removing jQuery from {{ghost_foot}} would break countless themes with no warning. This is a no-go.
  2. Add a jQuery="false" type attribute to {{ghost_foot}}
    This would sort of solve the problem, but it would leave us supporting this forever. It's also still opt-out. And it's really ugly. Let's not do that.
  3. Add a setting or config.js option
    As with no.2, we don't really want to be supporting this jQuery opt-out/in/up/down thing forever. We want to solve the problem once and for all.
  4. Move the inclusion of jQuery from {{ghost_foot}} into Code Injection
    Now that users can customise content output by {{ghost_foot}}, why not move the script tag there, rather than having it hardcoding into the helper itself? Then every user has the power to remove or leave it as they see fit.
  5. Leave it as it is
    Eurgh really?

So assuming we're all in agreement that 4 is the awesome solution we've all been waiting for, we first have to give kudos to @JohnONolan who thunk it up.

Now to the how:

ghost_foot is a row in the settings table, which stores anything that anyone puts in their Blog Footer section on the Code Injection screen.

The proposal is to write a special migration function which runs when anyone is upgrading 'through' a specific version of Ghost (i.e. their old version is lower, and their new version will be equal or higher). At the moment, the only system we have for this sort of thing exists for database migration. Therefore it may make sense to tie doing this to a database change (of which there are a few in the pipeline).

This function will insert the script tag for jQuery into the start of the ghost_foot field, with a newline after, and a comment before saying to only delete the line if the user is sure their theme doesn't require it. At the same time, the code can be removed from the {{ghost_foot}} helper.

Moving forward

All existing installed blogs will be unaffected by this change. jQuery will continue to be present, and so their themes will continue to work. Any theme which wasn't including the {{ghost_foot}} helper also won't get jQuery from Code Injection, so the net effect for existing blogs should be exactly 0.

All newly installed blogs however, will no longer have jQuery included & themes that require jQuery and do not include it will be missing functionality. To mitigate this we will start announcing that this change is going to happen on the dev blog and theme developer mailing lists. Theme developers will need to update their themes to include jQuery.

@ErisDS ErisDS added the themes label May 20, 2015
@JohnONolan
Copy link
Member

I approve of this issue title

@jaswilli
Copy link
Contributor

Option 4 sounds good to me. Are we talking no longer providing jquery.js from /public as well? 👍

@ErisDS
Copy link
Member Author

ErisDS commented May 21, 2015

Yeah I think it's less useful than using a CDN which is the norm, and where you can pick a version?

@ErisDS
Copy link
Member Author

ErisDS commented May 21, 2015

I guess the script tag we put in {{ghost_foot}} ought to link to a CDN rather than Ghost - then we can remove it 100%.

@jaswilli
Copy link
Contributor

Yep. That's what I was thinking. When doing the conversion, use a CDN for the code injection link and then we can also remove the plumbing where we're copying jquery.js out of the admin app into /public.

@JohnONolan
Copy link
Member

Linking to CDN requires disclosure in privacy.md and - as it's a retro-fitted change to existing installs - I don't think this is a good idea.

The reason it was self-hosted from the beginning was for privacy, not performance. We shouldn't make privacy-affecting changes to an existing feature, which users have no control over.

In this context it's not really a big deal - but the principal is important.

Can we think of any other way of handling this? I realise leaving jquery where it is isn't very desirable for forwards compatibility

@jaswilli
Copy link
Contributor

Good point. As part of the one-time conversion could we add a persistent notification linking to a relevant section in PRIVACY.md, or a separate document if it's only a temporary thing?

That way people who don't want the CDN version are made aware of the change and provided instructions for getting rid of it.

@ErisDS
Copy link
Member Author

ErisDS commented May 27, 2015

Because this impacts only existing users, I recommend doing the following:

  • use the http://code.jquery.com/ CDN, as it's provided by the jQuery Foundation, it's significantly less of a privacy issue than using a Google CDN.
  • display a notification/alert to anyone who has any of the existing privacy settings enabled - these are the users who need to know about this
  • disclose the change loudly and clearly in the release post / release page on GitHub

This should mean the message reaches the people who care, without concerning people who won't understand it or aren't affected.

@ErisDS ErisDS modified the milestone: Current Backlog Jun 2, 2015
@joeldrapper
Copy link

Number four sounds like a good solution however I think you should show a depreciation warning for a few versions if the installed themes rely on it and then remove it completely from fresh fresh installs later.

@ErisDS
Copy link
Member Author

ErisDS commented Jun 3, 2015

The way this is going to be implemented, it won't affect existing installs. Deprecation warnings don't work because there is nowhere to put it that guarantees it will get seen by the right people.

@SushiDude
Copy link

@ErisDS

Because this impacts only existing users, I recommend doing the following:
use the http://code.jquery.com/ CDN, as it's provided by the jQuery Foundation, it's significantly less of a privacy issue than using a Google CDN.

Do not do this. It is not just a privacy issue but rather a massive security issue; if the jQuery CDN were to be compromised, every Ghost blog would be compromised as well. Additionally, if a client visits any website that uses this CDN with a compromised session, a malicious version of jQuery could be served with an enormous cache time. This means that Ghost could be compromised even if it was not directly targeted. Pulling in third party assets, especially scripts, is a very bad idea.

Instead of using a CDN, just exclude jQuery from the new release tarballs. Users of existing installations can just delete the jQuery file along with the code injection if they see it fit.

acburdine added a commit to acburdine/Ghost that referenced this issue Aug 18, 2015
…hen migrating

closes TryGhost#5298
- remove all harcoded instances of jQuery throughout the front-end of the blog
- add migration function to add cdn link to ghost_foot code injection when migrating up from version 003
- migration version bump
sebgie pushed a commit to sebgie/Ghost that referenced this issue Aug 19, 2015
…hen migrating

closes TryGhost#5298
- remove all harcoded instances of jQuery throughout the front-end of the blog
- add migration function to add cdn link to ghost_foot code injection when migrating up from version 003
- migration version bump
ErisDS added a commit to ErisDS/Casper that referenced this issue Aug 24, 2015
refs TryGhost/Ghost#5298

- this if statement will include jQuery on the page, only if it isn't already included (i.e. in ghost_foot).
ErisDS added a commit to ErisDS/Casper that referenced this issue Aug 24, 2015
ErisDS added a commit to ErisDS/Casper that referenced this issue Aug 24, 2015
ErisDS added a commit to ErisDS/Casper that referenced this issue Aug 24, 2015
@ErisDS
Copy link
Member Author

ErisDS commented Aug 24, 2015

@SushiDude unfortunately simply removing jQuery from the release zip is not an option, as the Ghost upgrade process includes deleting all existing core files and replacing them with the new ones. Everyone would be left without jQuery with no way to know that had happened.

We already depend on the Google CDN, with the option to disable. Anyone concerned about using the jQuery CDN can remove the file from ghost_foot and add a local copy to their theme.

This change only affects existing blogs. New blogs will not have any reference to jQuery at all.

sebgie pushed a commit to sebgie/Ghost that referenced this issue Aug 31, 2015
closes TryGhost#5298
- remove all harcoded instances of jQuery throughout the front-end of the blog
- add migration function to add cdn link to ghost_foot code injection when migrating up from version 003
- migration version bump
ErisDS pushed a commit to ErisDS/Ghost that referenced this issue Sep 1, 2015
closes TryGhost#5298
- remove all harcoded instances of jQuery throughout the front-end of the blog
- add migration function to add cdn link to ghost_foot code injection when migrating up from version 003
- migration version bump
@SushiDude
Copy link

@ErisDS
#5787 would mitigate the worries about CDN security.

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

No branches or pull requests

5 participants