Skip to content

Commit

Permalink
Fix imports with a query part
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
simonihmig committed Dec 3, 2023
1 parent 79ab6ce commit d14ae9a
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 5 deletions.
5 changes: 4 additions & 1 deletion packages/ember-auto-import/ts/package.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<AddonInstance, Package> = new WeakMap();
Expand Down Expand Up @@ -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',
Expand Down
4 changes: 4 additions & 0 deletions packages/ember-auto-import/ts/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
35 changes: 31 additions & 4 deletions packages/ember-auto-import/ts/webpack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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'];

Expand All @@ -35,6 +37,27 @@ registerHelper('join', function (list, connector) {
return list.join(connector);
});

const moduleIdMap = new Map<string, string>();

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(){
Expand All @@ -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}}'); });
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -478,10 +505,10 @@ export default class WebpackBundler extends Plugin implements Bundler {
'utf8'
);
let outputSrc = inputSrc.replace(
/EAI_DISCOVERED_EXTERNALS\(['"]([^'"]+)['"]\)/g,
/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(',') + ']';
}
Expand Down

0 comments on commit d14ae9a

Please sign in to comment.