Upgrade from v6 to v7 sends digested assets to outside packs folder

[chuttam]

Note: Shakapacker v7.2.3 upgrade attempt from shakapacker v6.5.2 (currently working fine in production)

A discussion had previously been opened describing the same issue, but it was closed after discussion went into an irrelevant tangent around root cause, with no solutions offered.

Root Question

Why does running /bin/shakapacker with assets from npm modules create node_modules outside packs in v7, but creates node_modules folder within packs in v6?

---

Expected behavior:

When running bin/shakapacker, some 3rd-party assets (.eot, .woff, .woff2 font assets from font-awesome) are digested and placed in the following folder:

public/packs/node_modules/font-awesome  # <-- node_modules folder is a sub-folder in the packs folder.

This is the current, working behaviour with shakapacker v6.5.2. Browser access to https://my.asset.host/packs/node_modules/@fortawesome/fontawesome-pro/webfonts/fa-brands-400-2ac0cbf0ae3aa5e15f77.eot for example, resolves fine.

These assets are imported via the following SCSS file and appear to be picked up fine by the loader, and generate the pack manifest.

// Import Font Awesome
$fa-font-path: '~@fortawesome/fontawesome-pro/webfonts';   # <-- base URL variable can be set, for subsequent url(...) usage
@import '~@fortawesome/fontawesome-pro/scss/fontawesome';
@import '~@fortawesome/fontawesome-pro/scss/regular';
@import '~@fortawesome/fontawesome-pro/scss/light';
@import '~@fortawesome/fontawesome-pro/scss/solid';
@import '~@fortawesome/fontawesome-pro/scss/brands';

Actual behavior:

After upgrade to shakapacker v7.2.3, these assets are digested to a node_modules/ folder in public/ (i.e. outside the pack):

public/node_modules/font-awesome.  # <-- node_modules folder is now a peer folder of the packs folder.

The upgrade from shakapacker 6 to 7 is essentially the renaming of "webpacker" to "shakapacker". Here is a copy of the webpack config (identical to v6 version):

webpack.config.js

const { globalMutableWebpackConfig: webpackConfig } = require('shakapacker');
const webpack = require('webpack')
const { merge } = require('webpack-merge');
const WebpackAssetsManifest = require('webpack-assets-manifest');
const path = require('path');

// This section creates application.css as a webpack manifest asset.
// Requires use of css-loader and style-loader.
webpackConfig.module.rules.map((module) => {
  if (module.test && module.test.toString().includes('scss')) {
    module.use.splice(-1, 0, {
      loader: require.resolve('resolve-url-loader'),
    });
  }
  return module;
});

const options = {
  context: path.resolve(__dirname, '../../app/frontend'),    // <-- we renamed from the default 'javascript' convention.
};

module.exports = merge(options, webpackConfig);

shakapacker.yml

# Note: You must restart bin/webpack-dev-server for changes to take effect

default: &default
  source_path: app/frontend
  source_entry_path: packs
  public_root_path: public
  public_output_path: packs
  cache_path: tmp/cache/webpacker
  check_yarn_integrity: false
  webpack_compile_output: true

  # Additional paths webpack should lookup modules
  # ['app/assets', 'engine/foo/app/assets']
  additional_paths: ['vendor/dashkit']

  # Reload manifest.json on all requests so we reload latest compiled packs
  cache_manifest: false

  # Extract and emit a css file
  extract_css: true

development:
  <<: *default
  compile: true

  # Verifies that correct packages and versions are installed by inspecting package.json, yarn.lock, and node_modules
  check_yarn_integrity: true

  # Reference: https://webpack.js.org/configuration/dev-server/
  dev_server:
    https: false
    host: webpacker
    port: 3000
    public: cdn.easyredir.test
    hmr: false
    # Inline should be set to true if using HMR
    inline: true
    overlay: true
    compress: true
    live_reload: true
    disable_host_check: true
    use_local_ip: false
    quiet: false
    pretty: false
    headers:
      'Access-Control-Allow-Origin': '*'
    watch_options:
      ignored: '**/node_modules/**'
      aggregateTimeout: 300
      poll: 500
    client:
      overlay: false
      webSocketURL: "wss://cdn.easyredir.test/ws"
    # Note that apps that do not check the host are vulnerable to DNS rebinding attacks
    allowed_hosts: "all"
    static:
      watch:
        ignored: '**/node_modules/**'

test:
  <<: *default
  compile: true

  # Compile test packs to a separate directory
  public_output_path: packs-test

staging:
  <<: *default

  # Staging depends on precompilation of packs prior to booting for performance.
  compile: false

  # Cache manifest.json for performance
  cache_manifest: true

preview:
  <<: *default

  # Preview depends on precompilation of packs prior to booting for performance.
  compile: false

  # Cache manifest.json for performance
  cache_manifest: true

production:
  <<: *default

  # Production depends on precompilation of packs prior to booting for performance.
  compile: false

  # Cache manifest.json for performance
  cache_manifest: true

The generated manifest entries for the font assets appear technically correct:

 "../../node_modules/@fortawesome/fontawesome-pro/webfonts/fa-brands-400.eot": "https://my.assets.host/node_modules/@fortawesome/fontawesome-pro/webfonts/fa-brands-400-2ac0cbf0ae3aa5e15f77.eot",

However, any attempt to access the file in question is met with a 404 from our application, and these assets render as missing.

Small, reproducible repo: https://github.com/chuttam/shakapacker-scss-test

(run bin/shakapacker to see results in public/ and public/packs)

Setup environment:

• Ruby version: 3.1.6p260
• Rails version: 7.0.8.7
• Shakapacker version: Shakapacker v7.2.3 upgrade attempt from shakapacker v6.5.2 (currently working fine in production)

[chuttam]

Further to the statements above, it appears (to my untrained eye) that content in any folder above the packs/ folder (i.e. the public_output_path can't be served by webpack dev server. Is this accurate?

Edit: This statement appears true since dev_middleware can only have publicPath set to the config's public path (i.e. only packs/ and below).

[tomdracz]

Thanks for reporting. It looks like the crux of the issue is in this file

shakapacker/package/rules/file.js

Line 23 in 0829d5a

.filter(Boolean)

With your example, the resource filename ends up being something like ../../node_modules/@fontawesome... The fix would probably be stripping the relative path segments out.

Actually v6 behaviour seems to be incorrect also. I would have expected the static files to land in public/packs/static. The fact that they end up one level above is because in v6, only first segment was stripped from the path so it ended up being ../node_modules/@fontawesome...

Spend a good while tracing this as couldn't reproduce in my toy repo. Ended up tracing this to https://github.com/chuttam/shakapacker-scss-test/blob/main/config/webpack/webpack.config.js#L34 - that context line. Without it, the behaviour I see is correct. Which does make sense as the resolutions will be done against cwd rather than the nested directory and having to go up a tree.

If there's no concrete reason why you need the context declaration there, removing it is one way to go around this issue.

The other, in the meantime is redefining the rule in your webpack config to change the behaviour of the generator here:

shakapacker/package/rules/file.js

Line 11 in 0829d5a

generator: {

So it ends up with the key part being something like:

      const folders = path
        .replace(`${selectedStripPath}`, "")
        .split("/")
        .filter(Boolean)
        .filter(segment => segment !== '..')

      const foldersWithStatic = ["static", ...folders].join("/")
      return `${foldersWithStatic}/[name]-[hash][ext][query]`

[tomdracz]

Related Webpack issue webpack/webpack#14392

[tomdracz]

Looking into it more, it will definitely be more involved than just stripping relative segments from the path.

Say I have:
In Webpack config:
context: path.resolve(__dirname, '../../app/javascript'),
In Shakapacker config:
additional_paths: ['app/assets']
In one of the pack files:
import 'images/test_additional.svg'

And file that lives under app/assets/images/test_additional.svg

Then pathData.filename here

shakapacker/package/rules/file.js

Line 13 in 0829d5a

const path = dirname(pathData.filename)

will be relative to context so ../assets/images/test_additional.svg

And land in public/packs/static/assets/images/test_additional.svg

And without context set it would land in public/packs/static/images/test_additional.svg as we normally strip the additional path from the output.

So does need more thinking. As per linked Webpack issue it seems to be mainly due to behaviour difference between previous file-loader and new asset modules so would be helpful if Webpack itself provided something to support the use case here.

Thinking that one way to go about it is to try and resolve paths against process.cwd() and do some mashing but needs more investigation.

[chuttam]

@tomdracz First off, thank you for the diligence you've put into your responses. It definitely makes my days of effort crafting the question feel worthwhile and not futile.

My knee-jerk reaction would be to ideally remove the manual context declaration. Given this is an inherited project, I'd have to test the application extensively and get back to you whether this can be a successful resolution.

I may also attempt custom logic to strip out ".." segments VERY specifically for the font library in question. Thanks for tracking the underlying webpack issue down, by the way. I'd never have found it.

[tomdracz]

Thank you! Custom logic just for the fonts might be sensible.

To pinpoint any other issues with context removal, I'd suggest to diff the manifest.json file generated after compilation with and without the context configured. That should allow you to see if anything apart from the fonts ends up declared differently, if not, then should be safe to remove.

I'd expect this to only affect static file assets pulled from node_modules and additional paths outside the main source folder.

Got a good grasp on the scenarios now so will try and cook up some test cases than I can validate and fixes with, but will take some time.

Appreciate the report again, definitely something that we've not noticed around the Webpack change so at the very least I'll try and document this.

[tomdracz]

Doc update merged. Keeping this issue open as still exploring potential fixes

[chuttam]

@tomdracz Just confirming here that I was able to get my application to run successfully despite explicitly removing the context path option. A few lookup adjustments had to be made elsewhere, but all easily manageable. Third-party assets as asset/resource bundled successfully under packs/node_modules and are correctly referenced in the resulting manifest.

[tomdracz]

Great, thanks for the update @chuttam !