diff --git a/packages/compat/src/dependency-rules.ts b/packages/compat/src/dependency-rules.ts index fa077218a..5e3ffb6ac 100644 --- a/packages/compat/src/dependency-rules.ts +++ b/packages/compat/src/dependency-rules.ts @@ -51,6 +51,24 @@ export interface ComponentRules { // yieldsSafeComponents?: (boolean | { [name: string]: boolean })[]; + // This declares that our component yields some of its arguments unchanged. + // + // The array corresponds to your yielded positional arguments. Each value can + // be: + // false, meaning this yielded value is not one of our arguments + // a string, meaning this yielded value is our argument with that name + // or a POJO, whose individual properties are string naming which arguments + // from whence they came. + // + // Examples: + // + // If you do: {{yield @foo}} + // Then say: yieldsArguments: ['foo'] + // + // If you do: {{yield (hash x=@foo) }} + // Then say: yieldsArguments: [{ x: 'foo' }] + yieldsArguments?: (string | { [name: string]: string })[]; + // This declares that our component accepts arguments that will be invoked // with the {{component}} helper. This silences warnings in the places where // we consume them, while introducing warnings in the places where people are @@ -113,6 +131,7 @@ type ComponentSnippet = string; export interface PreprocessedComponentRule { yieldsSafeComponents: Required['yieldsSafeComponents']; + yieldsArguments: Required['yieldsArguments']; argumentsAreComponents: string[]; safeInteriorPaths: string[]; } @@ -142,6 +161,7 @@ export function preprocessComponentRule(componentRules: ComponentRules): Preproc argumentsAreComponents, safeInteriorPaths, yieldsSafeComponents: componentRules.yieldsSafeComponents || [], + yieldsArguments: componentRules.yieldsArguments || [], }; } diff --git a/packages/compat/src/resolver-transform.ts b/packages/compat/src/resolver-transform.ts index 1338e64f4..481207fdf 100644 --- a/packages/compat/src/resolver-transform.ts +++ b/packages/compat/src/resolver-transform.ts @@ -1,5 +1,4 @@ -import { default as Resolver } from './resolver'; -import { ComponentRules } from './dependency-rules'; +import { default as Resolver, ComponentResolution } from './resolver'; // This is the AST transform that resolves components and helpers at build time // and puts them into `dependencies`. @@ -15,14 +14,10 @@ export function makeResolverTransform(resolver: Resolver) { visitor: { Program: { enter(node: any) { - if (node.blockParams.length > 0) { - scopeStack.push(node.blockParams); - } + scopeStack.push(node.blockParams); }, - exit(node: any) { - if (node.blockParams.length > 0) { - scopeStack.pop(); - } + exit() { + scopeStack.pop(); }, }, BlockStatement(node: any) { @@ -39,17 +34,10 @@ export function makeResolverTransform(resolver: Resolver) { // a block counts as args from our perpsective (it's enough to prove // this thing must be a component, not content) let hasArgs = true; - let resolution = resolver.resolveMustache(node.path.original, hasArgs, env.moduleName); - if (resolution) { - if ( - resolution.type === 'component' && - node.program.blockParams.length > 0 && - resolution.yieldsComponents.length > 0 - ) { - scopeStack.yieldingComponents(resolution.yieldsComponents); - } - if (resolution.type === 'component') { - for (let name of resolution.argumentsAreComponents) { + const resolution = resolver.resolveMustache(node.path.original, hasArgs, env.moduleName); + if (resolution && resolution.type === 'component') { + scopeStack.enteringComponentBlock(resolution, ({ argumentsAreComponents }) => { + for (let name of argumentsAreComponents) { let pair = node.hash.pairs.find((pair: any) => pair.key === name); if (pair) { handleImpliedComponentHelper( @@ -62,7 +50,7 @@ export function makeResolverTransform(resolver: Resolver) { ); } } - } + }); } }, SubExpression(node: any) { @@ -110,27 +98,22 @@ export function makeResolverTransform(resolver: Resolver) { ElementNode: { enter(node: any) { if (!scopeStack.inScope(node.tag.split('.')[0])) { - let resolution = resolver.resolveElement(node.tag, env.moduleName); + const resolution = resolver.resolveElement(node.tag, env.moduleName); if (resolution && resolution.type === 'component') { - if (node.blockParams.length > 0 && resolution.yieldsComponents.length > 0) { - scopeStack.yieldingComponents(resolution.yieldsComponents); - } - for (let name of resolution.argumentsAreComponents) { - let attr = node.attributes.find((attr: any) => attr.name === '@' + name); - if (attr) { - handleImpliedComponentHelper(node.tag, name, attr.value, resolver, env.moduleName, scopeStack); + scopeStack.enteringComponentBlock(resolution, ({ argumentsAreComponents }) => { + for (let name of argumentsAreComponents) { + let attr = node.attributes.find((attr: any) => attr.name === '@' + name); + if (attr) { + handleImpliedComponentHelper(node.tag, name, attr.value, resolver, env.moduleName, scopeStack); + } } - } + }); } } - if (node.blockParams.length > 0) { - scopeStack.push(node.blockParams); - } + scopeStack.push(node.blockParams); }, - exit(node: any) { - if (node.blockParams.length > 0) { - scopeStack.pop(); - } + exit() { + scopeStack.pop(); }, }, }, @@ -144,9 +127,14 @@ export function makeResolverTransform(resolver: Resolver) { return resolverTransform; } -type ScopeEntry = - | { type: 'blockParams'; blockParams: string[] } - | { type: 'safeComponentMarker'; safeComponentMarker: Required['yieldsSafeComponents'] }; +interface ComponentBlockMarker { + type: 'componentBlockMarker'; + resolution: ComponentResolution; + argumentsAreComponents: string[]; + exit: (marker: ComponentBlockMarker) => void; +} + +type ScopeEntry = { type: 'blockParams'; blockParams: string[] } | ComponentBlockMarker; class ScopeStack { private stack: ScopeEntry[] = []; @@ -161,7 +149,9 @@ class ScopeStack { // by a safe component marker, we also clear that. pop() { this.stack.shift(); - if (this.stack.length > 0 && this.stack[0]!.type === 'safeComponentMarker') { + let next = this.stack[0]; + if (next && next.type === 'componentBlockMarker') { + next.exit(next); this.stack.shift(); } } @@ -169,8 +159,13 @@ class ScopeStack { // right before we enter a block, we might determine that some of the values // that will be yielded as marked (by a rule) as safe to be used with the // {{component}} helper. - yieldingComponents(safeComponentMarker: Required['yieldsSafeComponents']) { - this.stack.unshift({ type: 'safeComponentMarker', safeComponentMarker }); + enteringComponentBlock(resolution: ComponentResolution, exit: ComponentBlockMarker['exit']) { + this.stack.unshift({ + type: 'componentBlockMarker', + resolution, + argumentsAreComponents: resolution.argumentsAreComponents.slice(), + exit, + }); } inScope(name: string) { @@ -193,19 +188,39 @@ class ScopeStack { for (let i = 0; i < this.stack.length - 1; i++) { let here = this.stack[i]; let next = this.stack[i + 1]; - if (here.type === 'blockParams' && next.type === 'safeComponentMarker') { + if (here.type === 'blockParams' && next.type === 'componentBlockMarker') { let positionalIndex = here.blockParams.indexOf(parts[0]); if (positionalIndex === -1) { continue; } + if (parts.length === 1) { - return next.safeComponentMarker[positionalIndex] === true; + if (next.resolution.yieldsComponents[positionalIndex] === true) { + return true; + } + let sourceArg = next.resolution.yieldsArguments[positionalIndex]; + if (typeof sourceArg === 'string') { + next.argumentsAreComponents.push(sourceArg); + return true; + } } else { - let entry = next.safeComponentMarker[positionalIndex]; + let entry = next.resolution.yieldsComponents[positionalIndex]; if (entry && typeof entry === 'object') { return entry[parts[1]] === true; } + + let argsEntry = next.resolution.yieldsArguments[positionalIndex]; + if (argsEntry && typeof argsEntry === 'object') { + let sourceArg = argsEntry[parts[1]]; + if (typeof sourceArg === 'string') { + next.argumentsAreComponents.push(sourceArg); + return true; + } + } } + // we found the source of the name, but there were no rules to cover it. + // Don't keep searching higher, those are different names. + return false; } } return false; diff --git a/packages/compat/src/resolver.ts b/packages/compat/src/resolver.ts index d3f32b7c2..18ddc1b6b 100644 --- a/packages/compat/src/resolver.ts +++ b/packages/compat/src/resolver.ts @@ -16,19 +16,22 @@ import { Memoize } from 'typescript-memoize'; import { ResolvedDep } from '@embroider/core/src/resolver'; import resolve from 'resolve'; -type ResolutionResult = - | { - type: 'component'; - modules: ResolvedDep[]; - yieldsComponents: Required['yieldsSafeComponents']; - argumentsAreComponents: string[]; - } - | { - type: 'helper'; - modules: ResolvedDep[]; - }; +export interface ComponentResolution { + type: 'component'; + modules: ResolvedDep[]; + yieldsComponents: Required['yieldsSafeComponents']; + yieldsArguments: Required['yieldsArguments']; + argumentsAreComponents: string[]; +} + +export interface HelperResolution { + type: 'helper'; + modules: ResolvedDep[]; +} + +export type ResolutionResult = ComponentResolution | HelperResolution; -interface ResolutionFail { +export interface ResolutionFail { type: 'error'; hardFail: boolean; message: string; @@ -324,6 +327,7 @@ export default class CompatResolver implements Resolver { runtimeName: p.runtimeName, })), yieldsComponents: componentRules ? componentRules.yieldsSafeComponents : [], + yieldsArguments: componentRules ? componentRules.yieldsArguments : [], argumentsAreComponents: componentRules ? componentRules.argumentsAreComponents : [], }; } diff --git a/packages/compat/tests/renaming-tests.ts b/packages/compat/tests/renaming-tests.ts index dc0f8ce6f..a7b429efd 100644 --- a/packages/compat/tests/renaming-tests.ts +++ b/packages/compat/tests/renaming-tests.ts @@ -82,8 +82,14 @@ QUnit.module('renaming tests', function(origHooks) { (firstAddonWithAppTreeImport.files.app as Project['files'])[ 'first.js' ] = `export { default } from 'has-app-tree-import';`; + (firstAddonWithAppTreeImport.files.app as Project['files'])[ + 'imports-dep.js' + ] = `export { default } from 'inner-dep';`; (firstAddonWithAppTreeImport.files.addon as Project['files'])['index.js'] = `export default "first-copy";`; + let innerDep = firstAddonWithAppTreeImport.addAddon('inner-dep'); + (innerDep.files.addon as Project['files'])['index.js'] = `export default "inner-dep";`; + let secondAddonWithAppTreeImport = app.addAddon('intermediate').addAddon('has-app-tree-import'); (secondAddonWithAppTreeImport.files.app as Project['files'])[ 'second.js' @@ -168,4 +174,10 @@ QUnit.module('renaming tests', function(origHooks) { /export \{ default \} from ['"]\.\/node_modules\/intermediate\/node_modules\/has-app-tree-import['"]/ ); }); + test(`files copied into app from addons resolve the addon's deps`, function(assert) { + let assertFile = assert.file('imports-dep.js').transform(build.transpile); + assertFile.matches( + /export \{ default \} from ['"]\.\/node_modules\/has-app-tree-import\/node_modules\/inner-dep['"]/ + ); + }); }); diff --git a/packages/compat/tests/resolver-test.ts b/packages/compat/tests/resolver-test.ts index 6e4ebaf97..be4ba6dd3 100644 --- a/packages/compat/tests/resolver-test.ts +++ b/packages/compat/tests/resolver-test.ts @@ -803,6 +803,34 @@ QUnit.module('compat-resolver', function(hooks) { ); }); + test(`element block params are not in scope for element's own attributes`, function(assert) { + let packageRules = [ + { + package: 'the-test-package', + components: { + '': { + acceptsComponentArguments: ['title'], + yieldsSafeComponents: [true], + }, + }, + }, + ]; + let findDependencies = configure({ staticComponents: true, packageRules }); + givenFile('templates/components/form-builder.hbs'); + + assertWarning(/ignoring dynamic component title/, () => { + assert.deepEqual( + findDependencies('templates/application.hbs', ``), + [ + { + runtimeName: 'the-app/templates/components/form-builder', + path: './components/form-builder.hbs', + }, + ] + ); + }); + }); + test('acceptsComponentArguments on mustache with invalid literal', function(assert) { let packageRules = [ { @@ -960,4 +988,219 @@ QUnit.module('compat-resolver', function(hooks) { }, ]); }); + + test('respects yieldsArguments rule for positional block param, angle', function(assert) { + let packageRules: PackageRules[] = [ + { + package: 'the-test-package', + components: { + '': { + yieldsArguments: ['navbar'], + }, + }, + }, + ]; + let findDependencies = configure({ staticComponents: true, packageRules }); + givenFile('templates/components/form-builder.hbs'); + givenFile('templates/components/fancy-navbar.hbs'); + + assert.deepEqual( + findDependencies( + 'templates/components/x.hbs', + ` + + {{component bar}} + + ` + ), + [ + { + path: './fancy-navbar.hbs', + runtimeName: 'the-app/templates/components/fancy-navbar', + }, + { + path: './form-builder.hbs', + runtimeName: 'the-app/templates/components/form-builder', + }, + ] + ); + }); + + test('respects yieldsArguments rule for positional block param, curly', function(assert) { + let packageRules: PackageRules[] = [ + { + package: 'the-test-package', + components: { + '': { + yieldsArguments: ['navbar'], + }, + }, + }, + ]; + let findDependencies = configure({ staticComponents: true, packageRules }); + givenFile('templates/components/form-builder.hbs'); + givenFile('templates/components/fancy-navbar.hbs'); + + assert.deepEqual( + findDependencies( + 'templates/components/x.hbs', + ` + {{#form-builder navbar=(component "fancy-navbar") as |bar|}} + {{component bar}} + {{/form-builder}} + ` + ), + [ + { + path: './fancy-navbar.hbs', + runtimeName: 'the-app/templates/components/fancy-navbar', + }, + { + path: './form-builder.hbs', + runtimeName: 'the-app/templates/components/form-builder', + }, + ] + ); + }); + + test('respects yieldsArguments rule for hash block param', function(assert) { + let packageRules: PackageRules[] = [ + { + package: 'the-test-package', + components: { + '': { + yieldsArguments: [{ bar: 'navbar' }], + }, + }, + }, + ]; + let findDependencies = configure({ staticComponents: true, packageRules }); + givenFile('templates/components/form-builder.hbs'); + givenFile('templates/components/fancy-navbar.hbs'); + + assert.deepEqual( + findDependencies( + 'templates/components/x.hbs', + ` + + {{component f.bar}} + + ` + ), + [ + { + path: './fancy-navbar.hbs', + runtimeName: 'the-app/templates/components/fancy-navbar', + }, + { + path: './form-builder.hbs', + runtimeName: 'the-app/templates/components/form-builder', + }, + ] + ); + }); + + test('yieldsArguments causes warning to propagate up lexically, angle', function(assert) { + let packageRules: PackageRules[] = [ + { + package: 'the-test-package', + components: { + '': { + yieldsArguments: ['navbar'], + }, + }, + }, + ]; + let findDependencies = configure({ staticComponents: true, packageRules }); + givenFile('templates/components/form-builder.hbs'); + + assertWarning(/ignoring dynamic component this\.unknown/, () => { + assert.deepEqual( + findDependencies( + 'templates/components/x.hbs', + ` + + {{component bar}} + + ` + ), + [ + { + path: './form-builder.hbs', + runtimeName: 'the-app/templates/components/form-builder', + }, + ] + ); + }); + }); + + test('yieldsArguments causes warning to propagate up lexically, curl', function(assert) { + let packageRules: PackageRules[] = [ + { + package: 'the-test-package', + components: { + '': { + yieldsArguments: ['navbar'], + }, + }, + }, + ]; + let findDependencies = configure({ staticComponents: true, packageRules }); + givenFile('templates/components/form-builder.hbs'); + + assertWarning(/ignoring dynamic component this\.unknown/, () => { + assert.deepEqual( + findDependencies( + 'templates/components/x.hbs', + ` + {{#form-builder navbar=this.unknown as |bar|}} + {{component bar}} + {{/form-builder}} + ` + ), + [ + { + path: './form-builder.hbs', + runtimeName: 'the-app/templates/components/form-builder', + }, + ] + ); + }); + }); + + test('yieldsArguments causes warning to propagate up lexically, multiple levels', function(assert) { + let packageRules: PackageRules[] = [ + { + package: 'the-test-package', + components: { + '': { + yieldsArguments: ['navbar'], + }, + }, + }, + ]; + let findDependencies = configure({ staticComponents: true, packageRules }); + givenFile('templates/components/form-builder.hbs'); + + assertWarning(/ignoring dynamic component this\.unknown/, () => { + assert.deepEqual( + findDependencies( + 'templates/components/x.hbs', + ` + {{#form-builder navbar=this.unknown as |bar1|}} + {{#form-builder navbar=bar1 as |bar2|}} + {{component bar2}} + {{/form-builder}} + {{/form-builder}} + ` + ), + [ + { + path: './form-builder.hbs', + runtimeName: 'the-app/templates/components/form-builder', + }, + ] + ); + }); + }); }); diff --git a/packages/core/src/babel-plugin-adjust-imports.ts b/packages/core/src/babel-plugin-adjust-imports.ts index 79714d14d..804335cf3 100644 --- a/packages/core/src/babel-plugin-adjust-imports.ts +++ b/packages/core/src/babel-plugin-adjust-imports.ts @@ -179,6 +179,19 @@ function makeExternal(specifier: string, sourceFile: AdjustFile, opts: Options): return explicitRelative(dirname(sourceFile.name), target.slice(0, -3)); } +function handleRelocation(specifier: string, sourceFile: AdjustFile) { + let packageName = getPackageName(specifier); + if (!packageName) { + return specifier; + } + let pkg = sourceFile.owningPackage(); + if (!pkg || !pkg.isV2Ember()) { + return specifier; + } + let targetPkg = packageCache.resolve(packageName, pkg); + return explicitRelative(dirname(sourceFile.name), specifier.replace(packageName, targetPkg.root)); +} + function makeHBSExplicit(specifier: string, sourceFile: AdjustFile) { if (/\.(hbs)|(js)|(css)$/.test(specifier)) { // already has a well-known explicit extension, so nevermind @@ -270,6 +283,9 @@ export default function main({ types: t }: { types: any }) { let specifier = adjustSpecifier(source.value, file, opts); specifier = handleExternal(specifier, file, opts); + if (file.isRelocated) { + specifier = handleRelocation(specifier, file); + } specifier = makeHBSExplicit(specifier, file); if (specifier !== source.value) { emberCLIVanillaJobs.push(() => (source.value = specifier)); @@ -315,6 +331,10 @@ class AdjustFile { this.originalFile = relocatedFiles[name] || name; } + get isRelocated() { + return this.originalFile !== this.name; + } + @Memoize() owningPackage(): Package | undefined { return packageCache.ownerOfFile(this.originalFile); diff --git a/test-packages/node.test.js b/test-packages/node.test.js index 82bc3d79c..4775b9d6e 100644 --- a/test-packages/node.test.js +++ b/test-packages/node.test.js @@ -1,7 +1,7 @@ const execa = require('execa'); test('node', async () => { - jest.setTimeout(60000); + jest.setTimeout(1200000); await execa('yarn', ['node-test'], { cwd: `${__dirname}/..`,