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

Netlify CMS always includes Netlify Identity Widget in JS #9209

Closed
nakedible opened this issue Oct 18, 2018 · 10 comments
Closed

Netlify CMS always includes Netlify Identity Widget in JS #9209

nakedible opened this issue Oct 18, 2018 · 10 comments

Comments

@nakedible
Copy link
Contributor

Description

Netlify identity widget can be disabled for gatsby-plugin-netlify-cms by specifying enableIdentityWidget: false in configuration. I would expect that this would also ensure that the netlify identity widget would not be included in the built application JS files through WebPack. However, by using source-map-explorer that there is 161 kB of netlify-identity-widget present in the build.

Steps to reproduce

  1. Add netlify CMS plugin:
    {
      resolve: 'gatsby-plugin-netlify-cms',
      options: {
        enableIdentityWidget: false,
        modulePath: `${__dirname}/src/cms/cms.js`
      }
    },
  1. Build site npm run build
  2. Check if the resulting app has widget included: npx source-map-explorer public/app-*.js

Expected result

Netlify-identity-widget should be excluded if disabled.

Actual result

Netlify-identity-widget is included in JS bundle.

Environment

Cannot get info due to #8502

@pieh
Copy link
Contributor

pieh commented Oct 18, 2018

cc @erquhart - can we use null-loader or some other stuff to conditionally (not) include netlify-identity-widget in bundle?

@erquhart
Copy link
Contributor

This is mostly about how Netlify CMS itself is distributed - it currently includes all first party extensions, one of which is the Netlify Git Gateway backend. There's plenty of unecessary code there for sure, it's something we're definitely planning to improve.

That said, the Netlify CMS plugin creates and outputs it's own build straight to the public directory, along with generating it's own index.html, so your main site/bundle should not be impacted. I'm surprised tree shaking doesn't help here.

@pieh I haven't used null-loader, but if it works and doesn't break things, sounds good to me. Also wondering if maybe excluding the netlify cms module in the main site's webpack config could do the trick as well.

@pieh
Copy link
Contributor

pieh commented Oct 23, 2018

I guess it comes from here https://github.com/gatsbyjs/gatsby/blob/6e0a7ae4f5b0f965368d0a8b5e1ccf95240045f9/packages/gatsby-plugin-netlify-cms/src/gatsby-browser.js - importing it in gatsby-browser will include that in app bundle even if enableIdentityWidget will be set to false as this is variable that can't be resolved during build time - we could either use DefinePlugin so webpack can do buildtime optimization and tree shake that, or we can use null-loader to just make it so this import will be null

import netlifyIdentityWidget from "netlify-identity-widget"

I would go with null-loader approach as it seems easier.

Would You like to try creating PR for this @nakedible?

@erquhart
Copy link
Contributor

Ah, forget everything I said, it's that import, right.

Can Gatsby's webpack/babel setup handle dynamic imports? That'd be a pretty ideal approach here.

@pieh
Copy link
Contributor

pieh commented Oct 23, 2018

Yeah, you can do dynamic imports

@nakedible
Copy link
Contributor Author

nakedible commented Oct 24, 2018

Actually, I do not understand why netlify-identity-widget needs to be included for every page? Isn't that only needed for pages under the /admin/ path, which anyway has a separate javascript etc. setup? Or is there some use case where the identity widget would be required by Netlify CMS on the actual rendered pages?

I can make a PR, but I need to understand first the rationale behind the current choices.

To clarify: I don't understand why gatsby-browser.js exists at all in Netlify CMS plugin.

@erquhart
Copy link
Contributor

Many Netlify CMS users also use the identity widget. When you first sign up with the identity widget, you have to confirm your email, and the link in your email must send you to your site root. The widget must be running to catch you there with the auth token and forward you to your CMS page.

If Gatsby had a way to target specific pages with gatsby-browser we'd use it, but I don't think that's possible currently.

@nakedible
Copy link
Contributor Author

Okay, that means that for the user who does not use Netlify Identity on their website, but does wish to use Netlify Identity for Netlify CMS only, the only approach currently is to include extra 161 kB in every page load (or atleast for site root loads, which is about as bad).

So this is because the confirm emails are not directing the user to /admin/, but instead to the site root, the site root must handle netlify identity email confirmations. That is unfortunate.

I did some digging in the netlify-identity-widget internals, and noticed that these are things it processes:

const routes = /(confirmation|invite|recovery|email_change)_token=([^&]+)/;
const errorRoute = /error=access_denied&error_description=403/;
const accessTokenRoute = /access_token=/;

These are matched from the hash portion of the URI.

So, at least a few choices:

  1. Load 'netlify-identity-widget' only when a hash identifier matching the above is present in the URI.
  2. Redirect to /admin/ when a hash identifier matching the above is present in the URI, preserving the hash identifier (I assume the admin interface has the identity widget included).
  3. Somehow manage to affect the URL placed in the email confirmation to be something else than the site root (not sure if this is possible, but could inquire from the Netlify team, citing this use case).

But, not sure if I have to motivation to fix those (at least yet), because currently I don't use Netlify Identity at all, not even for Netlify CMS. I'll see about fixing the problem for my use case.

@erquhart
Copy link
Contributor

Dynamically loading when the hash is present could work.

@SumitBando
Copy link

According to Google Pagespeed Insights, this 49kb JS file is loading in a blocking fashion, and causing 1s of rendering delay. Not having this will be desirable.

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

4 participants