From db1ea5455f5244d9b9b4d7a46a157e94f87d8651 Mon Sep 17 00:00:00 2001 From: void_malex Date: Mon, 22 May 2023 21:37:27 +0100 Subject: [PATCH 01/12] add inner engine route add linkto to engine route --- .../engines-host-app/app/templates/application.hbs | 1 + tests/fixtures/engines-host-app/ember-cli-build.js | 8 +++++++- tests/fixtures/lazy-engine/addon/routes.js | 4 +++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/fixtures/engines-host-app/app/templates/application.hbs b/tests/fixtures/engines-host-app/app/templates/application.hbs index a77ba4321..1db8f8da7 100644 --- a/tests/fixtures/engines-host-app/app/templates/application.hbs +++ b/tests/fixtures/engines-host-app/app/templates/application.hbs @@ -1,6 +1,7 @@ diff --git a/tests/fixtures/engines-host-app/ember-cli-build.js b/tests/fixtures/engines-host-app/ember-cli-build.js index 78b35e984..e4881d343 100644 --- a/tests/fixtures/engines-host-app/ember-cli-build.js +++ b/tests/fixtures/engines-host-app/ember-cli-build.js @@ -27,5 +27,11 @@ module.exports = function (defaults) { } const Webpack = require('@embroider/webpack').Webpack; - return require('@embroider/compat').compatBuild(app, Webpack); + return require('@embroider/compat').compatBuild(app, Webpack, { + // staticAddonTestSupportTrees: true, + // staticAddonTrees: true, + // staticHelpers: true, + // staticModifiers: true, + // staticComponents: true, + }); }; diff --git a/tests/fixtures/lazy-engine/addon/routes.js b/tests/fixtures/lazy-engine/addon/routes.js index b43e5545f..1dc519d28 100644 --- a/tests/fixtures/lazy-engine/addon/routes.js +++ b/tests/fixtures/lazy-engine/addon/routes.js @@ -1,3 +1,5 @@ import buildRoutes from 'ember-engines/routes'; -export default buildRoutes(function() {}); +export default buildRoutes(function() { + this.route('inner-engine') +}); From a39cac1538118b3efee11739ea6815009679a593 Mon Sep 17 00:00:00 2001 From: void_malex Date: Mon, 22 May 2023 22:26:54 +0100 Subject: [PATCH 02/12] There is no route named use-lazy-engine.inner-engine --- tests/fixtures/engines-host-app/ember-cli-build.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/fixtures/engines-host-app/ember-cli-build.js b/tests/fixtures/engines-host-app/ember-cli-build.js index e4881d343..0f89122a3 100644 --- a/tests/fixtures/engines-host-app/ember-cli-build.js +++ b/tests/fixtures/engines-host-app/ember-cli-build.js @@ -28,8 +28,8 @@ module.exports = function (defaults) { const Webpack = require('@embroider/webpack').Webpack; return require('@embroider/compat').compatBuild(app, Webpack, { - // staticAddonTestSupportTrees: true, - // staticAddonTrees: true, + staticAddonTestSupportTrees: true, + staticAddonTrees: true, // staticHelpers: true, // staticModifiers: true, // staticComponents: true, From ac1d96eb52bfc892ba661249f72d919fba018d30 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 23 May 2023 09:50:16 -0400 Subject: [PATCH 03/12] proposed fix for engines router maps --- packages/compat/src/v1-addon.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/compat/src/v1-addon.ts b/packages/compat/src/v1-addon.ts index a887a3f24..38e25c385 100644 --- a/packages/compat/src/v1-addon.ts +++ b/packages/compat/src/v1-addon.ts @@ -1,5 +1,5 @@ import { Memoize } from 'typescript-memoize'; -import { dirname, join, relative } from 'path'; +import { dirname, join, relative, resolve } from 'path'; import { sync as pkgUpSync } from 'pkg-up'; import { existsSync, pathExistsSync } from 'fs-extra'; import buildFunnel, { Options as FunnelOptions } from 'broccoli-funnel'; @@ -732,7 +732,20 @@ export default class V1Addon { // .hbs.js templateExtensions: ['.hbs', '.hbs.js'], }); - if (!this.addonOptions.staticAddonTrees) { + if (this.addonOptions.staticAddonTrees) { + if (this.isEngine()) { + // even when staticAddonTrees is enabled, engines may have a router map + // that needs to be dynamically resolved. + let hasRoutesModule = false; + + tree = new ObserveTree(tree, outputDir => { + hasRoutesModule = existsSync(resolve(outputDir, 'routes.js')); + }); + built.dynamicMeta.push(() => ({ + 'implicit-modules': hasRoutesModule ? ['./routes.js'] : [], + })); + } + } else { let filenames: string[] = []; let templateOnlyComponentNames: string[] = []; From 023c08e824f59efc9ffca4367b6547c80a01860f Mon Sep 17 00:00:00 2001 From: void_malex Date: Tue, 23 May 2023 16:01:41 +0100 Subject: [PATCH 04/12] change to optional embroider setup inengine host app --- tests/fixtures/engines-host-app/ember-cli-build.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/tests/fixtures/engines-host-app/ember-cli-build.js b/tests/fixtures/engines-host-app/ember-cli-build.js index 0f89122a3..a0f16250f 100644 --- a/tests/fixtures/engines-host-app/ember-cli-build.js +++ b/tests/fixtures/engines-host-app/ember-cli-build.js @@ -26,12 +26,6 @@ module.exports = function (defaults) { return app.toTree(); } - const Webpack = require('@embroider/webpack').Webpack; - return require('@embroider/compat').compatBuild(app, Webpack, { - staticAddonTestSupportTrees: true, - staticAddonTrees: true, - // staticHelpers: true, - // staticModifiers: true, - // staticComponents: true, - }); + const { maybeEmbroider } = require('@embroider/test-setup'); + return maybeEmbroider(app); }; From 8710955131aaa11d6d86fefedd8059b9287ebb02 Mon Sep 17 00:00:00 2001 From: void_malex Date: Tue, 23 May 2023 16:02:32 +0100 Subject: [PATCH 05/12] add tests for embroider optimized code for engines --- tests/scenarios/engines-test.ts | 37 +++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/tests/scenarios/engines-test.ts b/tests/scenarios/engines-test.ts index 948cfa2a5..405c05d22 100644 --- a/tests/scenarios/engines-test.ts +++ b/tests/scenarios/engines-test.ts @@ -95,8 +95,23 @@ engineScenarios expectFile = expectFilesAt(readFileSync(join(app.dir, 'dist/.stage2-output'), 'utf8'), { qunit: assert }); }); - test(`pnpm test`, async function (assert) { - let result = await app.execute('pnpm test'); + test(`pnpm test safe`, async function (assert) { + let result = await app.execute('pnpm test', { + env: { + EMBROIDER_TEST_SETUP_OPTIONS: 'safe', + EMBROIDER_TEST_SETUP_FORCE: 'embroider', + }, + }); + assert.equal(result.exitCode, 0, result.output); + }); + + test(`pnpm test optimized`, async function (assert) { + let result = await app.execute('pnpm test', { + env: { + EMBROIDER_TEST_SETUP_OPTIONS: 'optimized', + EMBROIDER_TEST_SETUP_FORCE: 'embroider', + }, + }); assert.equal(result.exitCode, 0, result.output); }); @@ -130,11 +145,25 @@ engineScenarios app = await scenario.prepare(); }); - test(`pnpm test`, async function (assert) { - let result = await app.execute('pnpm test'); + test(`pnpm test safe`, async function (assert) { + let result = await app.execute('pnpm test', { + env: { + EMBROIDER_TEST_SETUP_OPTIONS: 'safe', + EMBROIDER_TEST_SETUP_FORCE: 'embroider', + }, + }); assert.equal(result.exitCode, 0, result.output); }); + test(`pnpm test optimized`, async function (assert) { + let result = await app.execute('pnpm test', { + env: { + EMBROIDER_TEST_SETUP_OPTIONS: 'optimized', + EMBROIDER_TEST_SETUP_FORCE: 'embroider', + }, + }); + assert.equal(result.exitCode, 0, result.output); + }); let visit: any; hooks.before(async () => { From e513fb86762d22d9be4cd61f5b35ecf692fcf379 Mon Sep 17 00:00:00 2001 From: void_malex Date: Mon, 29 May 2023 22:17:41 +0100 Subject: [PATCH 06/12] if the package is the engine itself then find and return it --- packages/core/src/module-resolver.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index 08e0fa645..764333b38 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -591,7 +591,9 @@ export class Resolver { // the app is always the first engine return this.options.engines[0]; } - let owningEngine = this.options.engines.find(e => e.activeAddons.find(a => a.root === pkg.root)); + let owningEngine = this.options.engines.find(e => + pkg.isEngine() ? e.root === pkg.root : e.activeAddons.find(a => a.root === pkg.root) + ); if (!owningEngine) { throw new Error( `bug in @embroider/core/src/module-resolver: cannot figure out the owning engine for ${pkg.root}` From 8b0e578ef9440440f02ad301cc0495d4aa46e2ee Mon Sep 17 00:00:00 2001 From: void_malex Date: Mon, 29 May 2023 22:23:33 +0100 Subject: [PATCH 07/12] this crates a flag for safe and optimized builds so tests can have bespoke forks but still check the same functionality applies --- .../tests/acceptance/basics-test.js | 72 +++++++++++-------- tests/scenarios/engines-test.ts | 7 +- 2 files changed, 49 insertions(+), 30 deletions(-) diff --git a/tests/fixtures/engines-host-app/tests/acceptance/basics-test.js b/tests/fixtures/engines-host-app/tests/acceptance/basics-test.js index 7b4a12cb4..2c6177cbf 100644 --- a/tests/fixtures/engines-host-app/tests/acceptance/basics-test.js +++ b/tests/fixtures/engines-host-app/tests/acceptance/basics-test.js @@ -16,24 +16,8 @@ function arrayOfCSSRules(styleSheets, cssSelector, cssProperty) { return values.sort(); } - -// We don't yet support lazy CSS in apps that are using fastboot. This test -// application runs both with and without fastboot. -const ensureCSSisLazy = !dependencySatisfies('ember-cli-fastboot', '*'); - -module('Acceptance | basics', function (hooks) { - setupApplicationTest(hooks); - - test('host-app', async function (assert) { - await visit('/'); - assert.equal(currentURL(), '/'); - assert.dom('[data-test-duplicated-helper]').containsText('from-engines-host-app'); - }); - - // this test must be the first test that loads the use-lazy-engine engine - // as after it has loaded it will not "unload" and we are checking that these - // modules are entering require.entries for the first time. - test('lazy-engine', async function (assert) { +function createLazyEngineTest(type) { + return async function (assert) { await visit('/'); let entriesBefore = Object.entries(window.require.entries).length; let rules = arrayOfCSSRules(document.styleSheets, '.shared-style-target', 'content'); @@ -77,7 +61,11 @@ module('Acceptance | basics', function (hooks) { await visit('/use-lazy-engine'); let entriesAfter = Object.entries(window.require.entries).length; - assert.ok(!!window.require.entries['lazy-engine/helpers/duplicated-helper']); + if (type === 'safe') { + assert.ok(!!window.require.entries['lazy-engine/helpers/duplicated-helper']); + } else { + assert.notOk(!!window.require.entries['lazy-engine/helpers/duplicated-helper']); + } assert.ok(entriesAfter > entriesBefore); assert.equal(currentURL(), '/use-lazy-engine'); assert.dom('[data-test-lazy-engine-main] > h1').containsText('Lazy engine'); @@ -118,15 +106,11 @@ module('Acceptance | basics', function (hooks) { '2px', 'lazy-engine vendor styles are present' ); - }); - - // See TODO comment in above test - //skip('lazy engines own app tree is lazy', function () {}); + }; +} - // this test must be the first test that loads the lazy-in-repo-engine as after it has loaded - // it will not "unload" and we are checkign that these modules are entering require.entries - // for the first time. - test('lazy-in-repo-engine', async function (assert) { +function createLazyInRepoEngineTest(type) { + return async function (assert) { await visit('/'); const entriesBefore = Object.entries(window.require.entries).length; let rules = arrayOfCSSRules(document.styleSheets, '.shared-style-target', 'content'); @@ -154,7 +138,11 @@ module('Acceptance | basics', function (hooks) { await visit('/use-lazy-in-repo-engine'); const entriesAfter = Object.entries(window.require.entries).length; - assert.ok(!!window.require.entries['lazy-in-repo-engine/helpers/duplicated-helper']); + if (type === 'safe') { + assert.ok(!!window.require.entries['lazy-in-repo-engine/helpers/duplicated-helper']); + } else { + assert.notOk(!!window.require.entries['lazy-in-repo-engine/helpers/duplicated-helper']); + } assert.ok(entriesAfter > entriesBefore); assert.equal(currentURL(), '/use-lazy-in-repo-engine'); assert.dom('[data-test-lazy-in-repo-engine-main] > h1').containsText('Lazy In-Repo Engine'); @@ -177,8 +165,36 @@ module('Acceptance | basics', function (hooks) { '2px', 'lazy-in-repo-engine addon styles are present' ); + }; +} +// We don't yet support lazy CSS in apps that are using fastboot. This test +// application runs both with and without fastboot. +const ensureCSSisLazy = !dependencySatisfies('ember-cli-fastboot', '*'); + +module('Acceptance | basics', function (hooks) { + setupApplicationTest(hooks); + + test('host-app', async function (assert) { + await visit('/'); + assert.equal(currentURL(), '/'); + assert.dom('[data-test-duplicated-helper]').containsText('from-engines-host-app'); }); + // this test must be the first test that loads the use-lazy-engine engine + // as after it has loaded it will not "unload" and we are checking that these + // modules are entering require.entries for the first time. + test('@safe lazy-engine', createLazyEngineTest('safe')); + test('@optimized lazy-engine', createLazyEngineTest('optimized')); + + // See TODO comment in above test + //skip('lazy engines own app tree is lazy', function () {}); + + // this test must be the first test that loads the lazy-in-repo-engine as after it has loaded + // it will not "unload" and we are checkign that these modules are entering require.entries + // for the first time. + test('@safe lazy-in-repo-engine', createLazyInRepoEngineTest('safe')); + test('@optimized lazy-in-repo-engine', createLazyInRepoEngineTest('optimized')); + test('eager-engine', async function (assert) { await visit('/use-eager-engine'); assert.equal(currentURL(), '/use-eager-engine'); diff --git a/tests/scenarios/engines-test.ts b/tests/scenarios/engines-test.ts index 405c05d22..bfeb5cf69 100644 --- a/tests/scenarios/engines-test.ts +++ b/tests/scenarios/engines-test.ts @@ -79,6 +79,7 @@ let engineScenarios = appScenarios.map('engines', project => { }); engineScenarios + .only('lts_3_28-engines') .map('without-fastboot', () => {}) .forEachScenario(scenario => { Qmodule(scenario.name, function (hooks) { @@ -96,7 +97,7 @@ engineScenarios }); test(`pnpm test safe`, async function (assert) { - let result = await app.execute('pnpm test', { + let result = await app.execute('pnpm test --filter=!@optimized', { env: { EMBROIDER_TEST_SETUP_OPTIONS: 'safe', EMBROIDER_TEST_SETUP_FORCE: 'embroider', @@ -106,7 +107,7 @@ engineScenarios }); test(`pnpm test optimized`, async function (assert) { - let result = await app.execute('pnpm test', { + let result = await app.execute('pnpm test --filter=!@safe', { env: { EMBROIDER_TEST_SETUP_OPTIONS: 'optimized', EMBROIDER_TEST_SETUP_FORCE: 'embroider', @@ -132,6 +133,8 @@ engineScenarios }); engineScenarios + .skip('lts_3_28-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 + .skip('lts_4_4-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 .skip('release-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 .map('with-fastboot', app => { app.linkDependency('ember-cli-fastboot', { baseDir: __dirname }); From 2931887e28159deb1675a812a606e3efcc137361 Mon Sep 17 00:00:00 2001 From: void_malex Date: Mon, 29 May 2023 23:16:56 +0100 Subject: [PATCH 08/12] test fix with lock file --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b89f05e4a..268699609 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,7 +53,7 @@ jobs: - uses: actions/checkout@v3 - uses: ./.github/actions/setup with: - use_lockfile: false + use_lockfile: true - name: suite run: ${{ matrix.command }} working-directory: ${{ matrix.dir }} From 6a5b8a2f2d048a5465d1501eff3c5ee33fbfb56c Mon Sep 17 00:00:00 2001 From: void_malex Date: Tue, 30 May 2023 11:25:45 +0100 Subject: [PATCH 09/12] re-enable tests --- tests/scenarios/engines-test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/scenarios/engines-test.ts b/tests/scenarios/engines-test.ts index bfeb5cf69..87869797f 100644 --- a/tests/scenarios/engines-test.ts +++ b/tests/scenarios/engines-test.ts @@ -79,7 +79,6 @@ let engineScenarios = appScenarios.map('engines', project => { }); engineScenarios - .only('lts_3_28-engines') .map('without-fastboot', () => {}) .forEachScenario(scenario => { Qmodule(scenario.name, function (hooks) { @@ -133,9 +132,6 @@ engineScenarios }); engineScenarios - .skip('lts_3_28-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 - .skip('lts_4_4-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 - .skip('release-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 .map('with-fastboot', app => { app.linkDependency('ember-cli-fastboot', { baseDir: __dirname }); app.linkDependency('fastboot', { baseDir: __dirname }); From f6b66d979c69a11628b2491d6709527a9f43634b Mon Sep 17 00:00:00 2001 From: void_malex Date: Tue, 30 May 2023 11:36:41 +0100 Subject: [PATCH 10/12] with correct filters for fastboot tests --- tests/scenarios/engines-test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/scenarios/engines-test.ts b/tests/scenarios/engines-test.ts index 87869797f..59e7ed91f 100644 --- a/tests/scenarios/engines-test.ts +++ b/tests/scenarios/engines-test.ts @@ -145,7 +145,7 @@ engineScenarios }); test(`pnpm test safe`, async function (assert) { - let result = await app.execute('pnpm test', { + let result = await app.execute('pnpm test --filter=!@optimized', { env: { EMBROIDER_TEST_SETUP_OPTIONS: 'safe', EMBROIDER_TEST_SETUP_FORCE: 'embroider', @@ -155,7 +155,7 @@ engineScenarios }); test(`pnpm test optimized`, async function (assert) { - let result = await app.execute('pnpm test', { + let result = await app.execute('pnpm test --filter=!@safe', { env: { EMBROIDER_TEST_SETUP_OPTIONS: 'optimized', EMBROIDER_TEST_SETUP_FORCE: 'embroider', From a51601285f19952768b2f7b35245a65832f8e980 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 30 May 2023 09:16:03 -0400 Subject: [PATCH 11/12] fixing type error revealed by new babel types --- .github/workflows/ci.yml | 2 +- packages/compat/src/audit/babel-visitor.ts | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 268699609..b89f05e4a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,7 +53,7 @@ jobs: - uses: actions/checkout@v3 - uses: ./.github/actions/setup with: - use_lockfile: true + use_lockfile: false - name: suite run: ${{ matrix.command }} working-directory: ${{ matrix.dir }} diff --git a/packages/compat/src/audit/babel-visitor.ts b/packages/compat/src/audit/babel-visitor.ts index caa6daa7c..441d82189 100644 --- a/packages/compat/src/audit/babel-visitor.ts +++ b/packages/compat/src/audit/babel-visitor.ts @@ -59,7 +59,7 @@ export function auditJS(rawSource: string, filename: string, babelConfig: Transf }, CallExpression(path: NodePath) { let callee = path.get('callee'); - if (callee.referencesImport('@embroider/macros', 'importSync') || t.isImport(callee)) { + if (callee.referencesImport('@embroider/macros', 'importSync') || t.isImport(callee.node)) { let arg = path.node.arguments[0]; if (arg.type === 'StringLiteral') { imports.push({ @@ -69,7 +69,9 @@ export function auditJS(rawSource: string, filename: string, babelConfig: Transf }); } else { problems.push({ - message: `audit tool is unable to understand this usage of ${t.isImport(callee) ? 'import' : 'importSync'}`, + message: `audit tool is unable to understand this usage of ${ + t.isImport(callee.node) ? 'import' : 'importSync' + }`, detail: arg.type, codeFrameIndex: saveCodeFrame(arg), }); From 8f5d445dc47b4c3a872cafb9d72464d48989101a Mon Sep 17 00:00:00 2001 From: void_malex Date: Tue, 30 May 2023 14:48:13 +0100 Subject: [PATCH 12/12] to get CI green --- tests/scenarios/engines-test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/scenarios/engines-test.ts b/tests/scenarios/engines-test.ts index 59e7ed91f..3b99c16f2 100644 --- a/tests/scenarios/engines-test.ts +++ b/tests/scenarios/engines-test.ts @@ -132,6 +132,9 @@ engineScenarios }); engineScenarios + .skip('lts_3_28-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 + .skip('lts_4_4-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 + .skip('release-engines') // fails due to https://github.com/emberjs/ember.js/pull/20461 .map('with-fastboot', app => { app.linkDependency('ember-cli-fastboot', { baseDir: __dirname }); app.linkDependency('fastboot', { baseDir: __dirname });