theme-scripts icon indicating copy to clipboard operation
theme-scripts copied to clipboard

theme-sections should include dynamic import logic

Open t-kelly opened this issue 5 years ago • 9 comments

Problem

In its current state, theme-sections is setup to load JS from all potential sections that show up on the home page, regardless if the merchants actually includes them or not in their layout. This means that unless the merchant uses every section available to them, we are loading in too much JS.

For example, a typical scripts/templates/index.js might statically imports and loads JS for all potential sections, regardless if those sections actually exist on the page or not:

import sections from './slate/sections';
window.theme.sections = sections;

// import all potential sections
import './sections/featured-blog';
import './sections/featured-product';
import './sections/featured-slider';
import './sections/list-collections';
import './sections/map-section';
import './sections/newsletter';
import './sections/quotes-section';
import './sections/slideshow-section';
import './sections/video-section';

$(document).ready(function() {
  templates.load('*');
  sections.load('*');
});

Solution

Theme-sections should be setup in a manor which allows section JS to be dynamically imported based on if it is present on the page or not.

A basic example of what checking the page before dynamically importing the JS for a section:

async function loadSection(type
  const sections = document.querySelectorAll(`[data-section-type="${type}"]`);

  if (sections.length > 0) {
    await import(/* webpackChunkName: type */ `section.${type}`); 
    return sections.load(type);
  }
}

loadSection('featured-slider');

t-kelly avatar Oct 31 '18 10:10 t-kelly

One way i currently get around this is:

  getElements('[data-module]').forEach(el => {
    const name = el.getAttribute('data-module')
    const Module = require(`../modules/${name}`).default
    new Module(el)
  })

On each section i'd add a data-module="carousel-section" and then it would dynamically load it instead of the upfront.

Would love to see this in theme-sections 🤙

dan-gamble avatar Oct 31 '18 10:10 dan-gamble

@t-kelly i tried implementing this with the slate.config.js and successfully got it creating the bundles. The only issue i had was when deployed Webpack was giving me a Cannot find module section.blah. Is this because Slate needs work to be able to handle these and once that's done, this can be achieved?

dan-gamble avatar Dec 20 '18 21:12 dan-gamble

@dan-gamble we need to be generating asset urls for each of these modules. Are you able to do that with your fix?

t-kelly avatar Jan 02 '19 15:01 t-kelly

Having played around with your way @t-kelly i realised my "Fix" wasn't a fix at all. It all gets bundled in to 1 file and is only conditionally executed as opposed to loaded.

I did get a decent way with what you had above but i think there was some Slate stuff getting in the way. I'll have another play and see if i can recreate where i got.

dan-gamble avatar Jan 02 '19 17:01 dan-gamble

Gave this another go to see if i could replicate my issue i was having @t-kelly.

So i started by expanding my slate.config.js with:

'webpack.entrypoints': config => {
  const entrypoints = {}

  // Module entrypoints
  fs.readdirSync(path.join(config.get('paths.theme.src.scripts'), 'modules')).forEach(file => {
    const name = path.parse(file).name
    const jsFile = path.join(
      config.get('paths.theme.src.scripts'),
      'modules',
      `${name}.js`
    )

    if (fs.existsSync(jsFile)) {
      entrypoints[`module.${name}`] = jsFile
    }
  })

  return entrypoints
}

This works fine:

Entrypoint layout.theme = layout.theme.css.liquid layout.theme.js layout.theme.js.map
Entrypoint template.gift_card = template.gift_card.js template.gift_card.js.map
Entrypoint template.product = template.product.js template.product.js.map
Entrypoint template.addresses = template.addresses.js template.addresses.js.map
Entrypoint template.login = template.login.js template.login.js.map
Entrypoint module.collection-per-page = module.collection-per-page.js module.collection-per-page.js.map
Entrypoint module.collection-sort-by = module.collection-sort-by.js module.collection-sort-by.js.map
Entrypoint module.drawers = module.drawers.js module.drawers.js.map
Entrypoint module.faqs-section = module.faqs-section.js module.faqs-section.js.map
Entrypoint module.push-cart = module.push-cart.js module.push-cart.js.map

I then have a loadModule function:

export async function loadModule (name) {
  return await import(/* webpackChunkName: name */ `module.${name}`)
}

Used like:

getElements('[data-module]').forEach(async el => {
  const name = el.getAttribute('data-module')
  console.log(await loadModule(name))
})

All this seems to work fine until i get to the browser where i'm faced with:

img

Done a bit of googling with not much luck, don't think it's a Slate issue per-se, probably more of a webpack one but i am a bit of a webpack noob

dan-gamble avatar Jan 05 '19 21:01 dan-gamble

To add to this. Changing the loadModule function to be:

await import(/* webpackChunkName: name */ '../modules/' + name + '.js')

Does seem to work. Maybe i've just not worked how to get it to load chunks properly.

dan-gamble avatar Jan 05 '19 22:01 dan-gamble

@t-kelly just curious to see if you knew any potential fixes to the above? Not wanting to accelerate this issue per-se, happy to have it in my slate.config until you can work your magic fingers with this one 🤙

dan-gamble avatar Jan 16 '19 09:01 dan-gamble

TBH I haven't had time to focus on the content of your exploration. Glancing through, your errors don't ring any bells. I need to wrap up https://github.com/Shopify/theme-scripts/pull/70. Will try and look at this next week.

t-kelly avatar Jan 16 '19 11:01 t-kelly

Don't want to put any pressure on you at all with this @t-kelly. Just wondered if you had any ideas off the top of your head, no worries at all that you don't. Looking forward to #70 🤙

dan-gamble avatar Jan 16 '19 13:01 dan-gamble