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

Fix imports with a query part #603

Merged
merged 2 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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];
}
33 changes: 30 additions & 3 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
Copy link
Contributor Author

@simonihmig simonihmig Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used for the argument to EAI_DISCOVERED_EXTERNALS(). When we have quotes in the query part of an import, mapping this part back to a module inside addDiscoveredExternals can become a bit more involved, because not only would we have to unescape the changes from js-string-escape, but in the case of devtool: 'eval' we would even end up with double-escaped quotes (' --js-string-escape--> \' --webpack-eval--> \\\').

So instead of trying to map this back to the original module specifier, I was just using something here that can serve as an identifier to the module, and which we can map back to it. In this case a md5 hash. Which has the benefit that we won't run into this kind of escaping/unescaping hell...

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 @@ -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(',') + ']';
}
Expand Down
48 changes: 46 additions & 2 deletions test-scenarios/static-import-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -132,6 +143,9 @@ function staticImportTest(project: Project) {
`,
},
},
assets: {
'test.specialfile': 'This is just plain text.',
},
},
tests: {
helpers: {
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -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'
);
});
});
`,
},
Expand Down Expand Up @@ -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 => {
Expand Down
Loading