From b51bc86887078e859e6357b68de1fdd836ffacd0 Mon Sep 17 00:00:00 2001 From: Simon Ihmig Date: Sun, 3 Dec 2023 20:33:13 +0100 Subject: [PATCH 1/2] Fix imports with a query part With the new `allowAppImports` feature, apps can import resources that are handledwith custom webpack loaders. Some can allow you to customize their behavior by passing query params, like `import image from './images/image.jpg?size=1024`. This PR is fixing two related issues: * make sure that an `allowAppImports` glob like `'images/**/*.jpg'` would also match for imports that include query params, by stripping the query part before trying to match the import to the glob * query params could technically also include quotes. These already got escaped when building the app-chunk that has the AMD-shims, but the regex to replace `EAI_DISCOVERED_EXTERNALS` was not taking their possible existance into account correctly. --- packages/ember-auto-import/ts/package.ts | 5 +++- packages/ember-auto-import/ts/util.ts | 4 +++ packages/ember-auto-import/ts/webpack.ts | 33 +++++++++++++++++++++--- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/packages/ember-auto-import/ts/package.ts b/packages/ember-auto-import/ts/package.ts index dd9ea3bb9..f4c9f51a9 100644 --- a/packages/ember-auto-import/ts/package.ts +++ b/packages/ember-auto-import/ts/package.ts @@ -13,6 +13,7 @@ import semver from 'semver'; import type { TransformOptions } from '@babel/core'; import { MacrosConfig } from '@embroider/macros/src/node'; import minimatch from 'minimatch'; +import { stripQuery } from './util'; // from child addon instance to their parent package const parentCache: WeakMap = new WeakMap(); @@ -315,7 +316,9 @@ export default class Package { if (!this.isAddon && packageName === this.name) { let localPath = path.slice(packageName.length + 1); if ( - this.allowAppImports.some((pattern) => minimatch(localPath, pattern)) + this.allowAppImports.some((pattern) => + minimatch(stripQuery(localPath), pattern) + ) ) { return { type: 'package', diff --git a/packages/ember-auto-import/ts/util.ts b/packages/ember-auto-import/ts/util.ts index 540f45f62..6859aca2f 100644 --- a/packages/ember-auto-import/ts/util.ts +++ b/packages/ember-auto-import/ts/util.ts @@ -6,3 +6,7 @@ export function shallowEqual(a: any[], b: any[]) { a.every((item, index) => item === b[index]) ); } + +export function stripQuery(path: string) { + return path.split('?')[0]; +} diff --git a/packages/ember-auto-import/ts/webpack.ts b/packages/ember-auto-import/ts/webpack.ts index 75709f579..5fa9e696a 100644 --- a/packages/ember-auto-import/ts/webpack.ts +++ b/packages/ember-auto-import/ts/webpack.ts @@ -8,6 +8,7 @@ import type { Module, } from 'webpack'; import { join, dirname, resolve, relative, posix, isAbsolute } from 'path'; +import { createHash } from 'crypto'; import { mergeWith, flatten, zip } from 'lodash'; import { writeFileSync, realpathSync, readFileSync } from 'fs'; import { compile, registerHelper } from 'handlebars'; @@ -25,6 +26,7 @@ import { ensureDirSync, symlinkSync, existsSync } from 'fs-extra'; import MiniCssExtractPlugin from 'mini-css-extract-plugin'; import minimatch from 'minimatch'; import { TransformOptions } from '@babel/core'; +import { stripQuery } from './util'; const EXTENSIONS = ['.js', '.ts', '.json']; @@ -35,6 +37,27 @@ registerHelper('join', function (list, connector) { return list.join(connector); }); +const moduleIdMap = new Map(); + +function moduleToId(moduleSpecifier: string) { + let id = moduleSpecifier; + + // if the module contains characters that need to be escaped, then map this to a hash instead, so we can easily replace this later + if (moduleSpecifier.includes('"') || moduleSpecifier.includes("'")) { + id = createHash('md5').update('some_string').digest('hex'); + + moduleIdMap.set(id, moduleSpecifier); + } + + return id; +} + +function idToModule(id: string) { + return moduleIdMap.get(id) ?? id; +} + +registerHelper('module-to-id', moduleToId); + const entryTemplate = compile( ` module.exports = (function(){ @@ -52,7 +75,7 @@ module.exports = (function(){ return r('_eai_sync_' + specifier)(Array.prototype.slice.call(arguments, 1)) }; {{#each staticImports as |module|}} - d('{{js-string-escape module.specifier}}', EAI_DISCOVERED_EXTERNALS('{{js-string-escape module.specifier}}'), function() { return require('{{js-string-escape module.specifier}}'); }); + d('{{js-string-escape module.specifier}}', EAI_DISCOVERED_EXTERNALS('{{module-to-id module.specifier}}'), function() { return require('{{js-string-escape module.specifier}}'); }); {{/each}} {{#each dynamicImports as |module|}} d('_eai_dyn_{{js-string-escape module.specifier}}', [], function() { return import('{{js-string-escape module.specifier}}'); }); @@ -368,7 +391,11 @@ export default class WebpackBundler extends Plugin implements Bundler { // Handling full-name imports that point at the app itself e.g. app-name/lib/thingy if (name === this.opts.rootPackage.name) { - if (this.importMatchesAppImports(request.slice(name.length + 1))) { + if ( + this.importMatchesAppImports( + stripQuery(request.slice(name.length + 1)) + ) + ) { // webpack should handle this because it's another file in the app that matches allowAppImports return callback(); } else { @@ -481,7 +508,7 @@ export default class WebpackBundler extends Plugin implements Bundler { /EAI_DISCOVERED_EXTERNALS\(['"]([^'"]+)['"]\)/g, (_substr: string, matched: string) => { let deps = build - .externalDepsFor(matched) + .externalDepsFor(idToModule(matched)) .filter((dep) => this.externalizedByUs.has(dep)); return '[' + deps.map((d) => `'${d}'`).join(',') + ']'; } From 12e5c09081537ac61568dbd6a5a77cde2be97b7a Mon Sep 17 00:00:00 2001 From: Simon Ihmig Date: Wed, 6 Dec 2023 19:38:25 +0100 Subject: [PATCH 2/2] Add tests for asset loading and query params --- test-scenarios/static-import-test.ts | 48 ++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/test-scenarios/static-import-test.ts b/test-scenarios/static-import-test.ts index fb0f69307..3f1b81b35 100644 --- a/test-scenarios/static-import-test.ts +++ b/test-scenarios/static-import-test.ts @@ -42,8 +42,19 @@ function staticImportTest(project: Project) { 'original-package' ], allowAppImports: [ - 'lib/**' - ] + 'lib/**', + 'assets/*.specialfile', + ], + webpack: { + module: { + rules: [ + { + test: /\.specialfile/, + use: 'specialfile-loader', + }, + ], + }, + }, } }); return app.toTree(); @@ -132,6 +143,9 @@ function staticImportTest(project: Project) { `, }, }, + assets: { + 'test.specialfile': 'This is just plain text.', + }, }, tests: { helpers: { @@ -200,6 +214,8 @@ function staticImportTest(project: Project) { dont_find_me_4, secret_string_7 } from '../../lib/example2'; + import testAsset from '@ef4/app-template/assets/test.specialfile'; + import { query as testAssetQuery } from '@ef4/app-template/assets/test.specialfile?foo=bar'; import Service from '@ember/service'; import example6Direct, { dont_find_me } from '@ef4/app-template/utils/example6'; import example7Direct, { secret_string } from '@ef4/app-template/utils/example7'; @@ -280,6 +296,20 @@ function staticImportTest(project: Project) { 'should not have example3 in loader' ); }); + test('it can import assets handled by loader', function (assert) { + assert.strictEqual( + testAsset, + 'This is just plain text.', + 'Content loaded from customloader can be imported' + ); + }); + test('it can import assets that have query params', function (assert) { + assert.strictEqual( + testAssetQuery, + '?foo=bar', + 'query params are correctly handled' + ); + }); }); `, }, @@ -312,6 +342,20 @@ function staticImportTest(project: Project) { }`, }, }); + + project.addDevDependency('specialfile-loader', { + files: { + 'index.js': ` + export default function customLoader(source) { + return \`export const query = \${JSON.stringify(this.resourceQuery)}; + export default \${JSON.stringify(source)}\`; + } + `, + 'package.json': JSON.stringify({ + type: 'module', + }), + }, + }); } let scenarios = appScenarios.map('static-import', project => {