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

Webpack dependency plugin: Fix filename function handling #15430

Merged
merged 3 commits into from May 6, 2019

Conversation

@sirreal
Copy link
Member

commented May 4, 2019

Description

As described in #15410, webpack accepts either a string or function for its output.filename configuration. @wordpress/dependency-extraction-webpack-plugin would error when a function filename was passed.

This was caused by attempting to replace the .js extension with .deps.json on the filename option string. The fix was to move the .js extension replacement to the result of compilation.getPath. This will correctly handle function filenames and allow replace to be called reliably on the string.

  • Add a test for webpack configuration with a function filename.
  • Apply the fix (call .replace on the result of compilation.getPath instead of the input).
  • Add new test snapshots.

Fixes #15410

How has this been tested?

Includes tests.

Types of changes

Bug fix in the unpublished @wordpress/dependency-extraction-webpack-plugin package.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

sirreal added some commits May 3, 2019

Move .js to .deps.json replacement to path result
Replacement in the output.filename template works fine with a string
filename. However, webpack also accepts a function filename, which
causes errors when the replacement is attempted.

Move the replacement to the result of `getPath`, when we should reliably
have a string.

Fixes #15410
@davilera

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Thanks @sirreal. This works like a charm.

The only problem I have now is, .deps.json does not include wp-* dependencies.

This is my config:

const config = {
  ...defaultConfig,
  plugins: [
    ...defaultConfig.plugins,
    new WordPressExternalDependenciesPlugin( {
      requestToExternal: ( request ) => …,
      requestToHandle: ( request ) => …,
    } ),
    new WebpackRTLPlugin( {
      suffix: '-rtl',
      minify: isProduction ? { safe: true } : false,
    } ),
  ],
};

Obviously, in requestToExternal and requestToHandle I manage my own modules and return undefined if the request is not "mine". Something like this;

( request ) => request.startsWith( NAB_NAMESPACE ) ?
  whatever... :
  undefined,

Should we address this issue in this PR, or should I open a new issue and ask for a new PR?

@davilera

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

OK, so the problem with wp-* dependencies has nothing to do with this PR. Apparently, I was using the version of wp-script published in npm, which wasn't compatible with the dependency extractor yet.

As I said before, I confirm this PR solves the issue I first posted in #15410.

@gziolo

gziolo approved these changes May 6, 2019

Copy link
Member

left a comment

Yes, this change makes sense. Good catch 👍

I think that added tests are enough to confirm that this issue is fixed :shipit:

@sirreal sirreal merged commit 4021055 into master May 6, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@sirreal sirreal deleted the fix/15410-webpackdepsplugin-outputfunc branch May 6, 2019

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.