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

A case for keeping around require.ensure #5909

Closed
noahgrant opened this Issue Nov 1, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@noahgrant
Copy link

noahgrant commented Nov 1, 2017

Do you want to request a feature or report a bug?
I'm requesting not deprecating an existing feature, require.ensure.

What is the current behavior?

If the current behavior is a bug, please provide the steps to reproduce.

What is the expected behavior?

If this is a feature request, what is motivation or use case for changing the behavior?
require.ensure is listed as the legacy method of code-splitting in the documentation, favoring instead the new dynamic imports spec. Currently, both work, but as require.ensure is legacy, it's just a matter of time before it gets deprecated. But dynamic imports have no ability to batch 'sibling' imports together into a common split (something I personally do often).

For example, let's say I have a Profile Page, a Billing Page, and a Settings Page that all belong under an Account Navigation tab. Each individual page is fairly small (let's say ~10kB gzipped), so I'd prefer to batch them together so that the user doesn't have to request a new javascript file on every page click—just once when they go anywhere in the Account tab (since we can likely fit all three within the 1 second load time we have per the RAIL model).

I might organize them like this:

require.ensure([
  // all three of these are siblings
  'path/to/profile_page',
  'path/to/billing_page',
  'path/to/settings_page'
], () => {
  // require currently-requested page
});

I could create a new parent file, account.js, that would simply import the three siblings and become a single parent node:

// account.js
import ProfilePage from 'path/to/profile_page';
import BillingPage from 'path/to/billing_page';
import SettingsPage from 'path/to/settings_page';

which would allow me to use:

import('path/to/account').then(() => {
  // set currently-requested page
});

but I'd prefer not to create a new file just for this purpose every time I want to batch imports. @TheLarkInn, I believe you mentioned that something like this would work by using magic comments to force the siblings into the same split:

Promise.all([
  import(/* webpackChunkName: 'account' */ 'path/to/profile_page'),
  import(/* webpackChunkName: 'account' */ 'path/to/billing_page'),
  import(/* webpackChunkName: 'account' */ 'path/to/settings_page')
]).then(([ProfilePage, BillingPage, SettingsPage]) => {
});

which I am happy to do, but at that point aren't we still relying on webpack-specific wrappers around dynamic importing (to support the magic comment) as opposed to adhering to spec?

And so, my real question then is: what is the best long-term solution for batching dynamic imports?

Thank you all for your time.

Please mention other relevant information such as the browser version, Node.js version, webpack version and Operating System.

@sokra

This comment has been minimized.

Copy link
Member

sokra commented Nov 2, 2017

That's the long term solution:

I could create a new parent file, account.js, that would simply import the three siblings and become a single parent node:

The spec doesn't provide a way to import multiple modules.

You could propose import(["a", "b", "c"]) to the spec, I would favor it.

@noahgrant

This comment has been minimized.

Copy link

noahgrant commented Nov 3, 2017

Okay, I wasn't able to get the 'forcing into the same split' solution to work on a first pass, but assuming that it does, I just have one follow-up question before closing this issue:

With the current require.ensure semantics, there is a clear distinction between an error in the script network request, and an error in the script execution:

    // imagine wrapping fn contains a pageKey variable
    require.ensure([
      'path/to/profile_page',
      'path/to/billing_page',
      'path/to/settings_page'
    ], () => {
      try {
        if (pageKey === 'profile') {
          page = require('path/to/profile_page');
        } else if (pageKey === 'billing') {
          page = require('path/to/billing_page');
        } else if (pageKey === 'settings') {
          page = require('path/to/settings_page');
        } else {
          return logNoPageMessage(pageKey, 'account');
        }

        cb(page, pageKey);
      } catch (err) {
        // here we know an error occurred in the _script execution_
        console.error(err);
      }
    // this reload function will be called when an error occurred in the _network request_, ie,
    // they've requested a code split with a hash that no longer exists, and we should reload
    // the page to give them the latest version of the javascript.
    }, reload, 'account');

I don't currently see a way with the import semantics to distinguish between these two classes of errors, unless I'm missing something?

Thanks again for your time.

@sokra

This comment has been minimized.

Copy link
Member

sokra commented Nov 5, 2017

I don't currently see a way with the import semantics to distinguish between these two classes of errors, unless I'm missing something?

You can inspect the Error object.


Anyway, require.ensure/CommonJS is not deprecated (yet) so I guess this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment