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.

Sign up for GitHub

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

Jump to bottom

Back onto real canary releases #2073

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Back onto real canary releases #2073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd15867
Back onto real canary releases
ef4 Aug 22, 2024
c32cffd
cleaning up ember 6.0 api removals in test suite
ef4 Aug 22, 2024
4637911
remove unsupported non-colocated component template
ef4 Aug 23, 2024
e1f9c1e
make CommandWatcher dump logs at shutdown() when a command has failed
ef4 Aug 23, 2024
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
4 changes: 2 additions & 2 deletions packages/macros/tests/glimmer/macro-maybe-attrs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ describe(`macroMaybeAttrs`, function () {

test('macroMaybeAttrs leaves other modifiers alone', function () {
let code = transform(
`<div data-test-target {{action this.doThing}} {{macroMaybeAttrs false data-optional data-flavor="vanilla" }} ></div>`
`<div data-test-target {{some-modifier this.doThing}} {{macroMaybeAttrs false data-optional data-flavor="vanilla" }} ></div>`
);
expect(code).toMatch(/<div data-test-target {{action this.doThing}}/);
expect(code).toMatch(/<div data-test-target {{some-modifier this.doThing}}/);
});
});
});
120 changes: 56 additions & 64 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 1 addition & 4 deletions test-packages/sample-transforms/tests/dummy/config/environment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@ module.exports = function (environment) {
// Here you can enable experimental features on an ember canary build
// e.g. EMBER_NATIVE_DECORATOR_SUPPORT: true
},
EXTEND_PROTOTYPES: {
// Prevent Ember Data from overriding Date.parse.
Date: false,
},
EXTEND_PROTOTYPES: false,
},

APP: {
Expand Down
5 changes: 1 addition & 4 deletions tests/fixtures/fastboot-app/config/environment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@ module.exports = function (environment) {
// Here you can enable experimental features on an ember canary build
// e.g. EMBER_NATIVE_DECORATOR_SUPPORT: true
},
EXTEND_PROTOTYPES: {
// Prevent Ember Data from overriding Date.parse.
Date: false,
},
EXTEND_PROTOTYPES: false,
},
fastboot: {
hostWhitelist: ['localhost:4200'],
Expand Down
5 changes: 1 addition & 4 deletions tests/fixtures/macro-test/config/environment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@ module.exports = function (environment) {
// Here you can enable experimental features on an ember canary build
// e.g. EMBER_NATIVE_DECORATOR_SUPPORT: true
},
EXTEND_PROTOTYPES: {
// Prevent Ember Data from overriding Date.parse.
Date: false,
},
EXTEND_PROTOTYPES: false,
},

APP: {
Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/macro-test/tests/integration/components/macro-if-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ module('Integration | Macro | macroCondition', function (hooks) {
assert.ok(true, 'it ran');
};
await render(
hbs`<div data-test-target {{action this.doThing}} {{macroMaybeAttrs false data-optional data-flavor="vanilla" }} ></div>`
hbs`<div data-test-target {{on "click" this.doThing}} {{macroMaybeAttrs false data-optional data-flavor="vanilla" }} ></div>`
);
let target = this.element.querySelector('[data-test-target]');
await click(target);
Expand Down
8 changes: 2 additions & 6 deletions tests/scenarios/compat-route-split-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,7 @@ splitScenarios
EmberENV: {
FEATURES: {
},
EXTEND_PROTOTYPES: {
Date: false
}
EXTEND_PROTOTYPES: false,
},
APP: {}
};
Expand Down Expand Up @@ -503,9 +501,7 @@ splitScenarios
EmberENV: {
FEATURES: {
},
EXTEND_PROTOTYPES: {
Date: false
}
EXTEND_PROTOTYPES: false,
},
APP: {}
};
Expand Down
33 changes: 6 additions & 27 deletions tests/scenarios/compat-stage2-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,20 +389,11 @@ stage2Scenarios
<HelloWorld @useDynamic="first-choice" />
<HelloWorld @useDynamic={{"second-choice"}} />
<HelloWorld @useDynamic={{component "third-choice"}} />
<DirectTemplateReexport />
`,
'curly.hbs': `
{{hello-world useDynamic="first-choice" }}
{{hello-world useDynamic=(component "third-choice") }}
`,
components: {
'first-choice.hbs': 'first',
'second-choice.hbs': 'second',
'third-choice.hbs': 'third',
'module-name-check': {
'index.hbs': '<div class={{embroider-sample-transforms-module}}>hello world</div>',
},
},
},
components: {
'uses-inline-template.js': `
Expand All @@ -411,6 +402,12 @@ stage2Scenarios
layout: hbs${'`'}<FirstChoice/>${'`'}
})
`,
'first-choice.hbs': 'first',
'second-choice.hbs': 'second',
Copy link
Contributor Author

@ef4 ef4 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is needed to eliminate non-colocated component usage, but it's causing failures because we currently have a real issue:

Extension search works in depscanning and in normal usage, but not in the dep pre-bundling that happens after depscanning. In this test, an addon uses global template resolution to access this component, which is failing because there's no hbs search.

It's unclear whether esbuild / vite will do a good or bad thing after we fix this, in that we definitely don't want pre-bundled dependencies pulling in any parts of the app.

Copy link
Contributor

@patricklx patricklx Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I also saw this and which is why i needed to do this change
Anyway the tests here in stage2 might need more changes. Maybe in a way that make them independent of vite dep optimization. Which is currently not happening. See this file

Edit: in this case it's actually accessing the app component?
The way here is to externalize it during bundling:
https://github.com/embroider-build/embroider/pull/1876/files#diff-a6c8071691a61acc68be864b34d3f338fd92e9bf5217c2d12403ac6029bca885R58

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we will want to externalize during bundling. But not exactly in the way the code you linked is doing it.

That code is using rough heuristics for isInApp that are unlikely to be long-term true. We have more precise tracking already to know which modules are in the app.

And when we externalize, we should do it via the request.specifier, not the resolution.filename. Because the way the request maps to the resolution can change during development, and the pre-bundled deps would not react to that.

Copy link
Contributor

@patricklx patricklx Aug 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are a few problems with that. The request is coming from an optimised dep and then the embroider resolution will not happen. It would also be hard to trace the dep chunk back to the origin.
What can be done is that we create a new specifier which includes both fromFile and specifier. I will try that out.

'third-choice.hbs': 'third',
'module-name-check': {
'index.hbs': '<div class={{embroider-sample-transforms-module}}>hello world</div>',
},
},
'use-deep-addon.js': `import thing from 'deep-addon'`,
'custom-babel-needed.js': `console.log('embroider-sample-transforms-target');`,
Expand Down Expand Up @@ -488,9 +485,6 @@ stage2Scenarios
},
templates: {
'app-example.hbs': `{{component this.stuff}}`,
components: {
'direct-template-reexport.js': `export { default } from 'my-addon/templates/components/hello-world';`,
},
},
},
public: {
Expand Down Expand Up @@ -690,21 +684,6 @@ stage2Scenarios
});
});

test('app/templates/components/direct-template-reexport.js', function (assert) {
expectAudit
.module('./templates/index.hbs')
.resolves(/my-addon\/_app_\/templates\/components\/direct-template-reexport\.js\/-embroider-pair-component/)
.toModule()
.resolves(/my-addon\/_app_\/templates\/components\/direct-template-reexport\.js/)
.toModule()
.withContents(contents => {
assert.ok(
/export { default } from ".*\/my-addon\/templates\/components\/hello-world.hbs.*";/.test(contents)
);
return true;
});
});

test('uses-inline-template.js', function (assert) {
expectAudit
.module('./components/uses-inline-template.js')
Expand Down
4 changes: 1 addition & 3 deletions tests/scenarios/compat-template-colocation-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,9 +332,7 @@ appScenarios
EmberENV: {
FEATURES: {
},
EXTEND_PROTOTYPES: {
Date: false
}
EXTEND_PROTOTYPES: false,
},
APP: {}
};
Expand Down
9 changes: 9 additions & 0 deletions tests/scenarios/helpers/command-watcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ export default class CommandWatcher {

async shutdown(): Promise<void> {
if (this.exitCode != null) {
this.maybeEmitLogs();
return;
}

Expand All @@ -119,6 +120,14 @@ export default class CommandWatcher {
});

await this.waitForExit();
this.maybeEmitLogs();
}

private maybeEmitLogs() {
if (this.exitCode !== 0) {
console.error(`CommandWatcher saw non-zero exit, dumping logs:`);
console.error(this.lines.join('\n'));
}
}

async waitForExit(): Promise<number> {
Expand Down
4 changes: 2 additions & 2 deletions tests/scenarios/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
"ember-source-5.4": "npm:ember-source@~5.4.0",
"ember-source-5.8": "npm:ember-source@~5.8.0",
"ember-source-beta": "npm:ember-source@beta",
"ember-source-canary": "https://s3.amazonaws.com/builds.emberjs.com/canary/shas/370cf34f9e86df17b880f11fef35a5a0f24ff38a.tgz",
"ember-source-canary": "npm:ember-source@alpha",
"ember-source-latest": "npm:ember-source@latest",
"ember-test-helpers-2": "npm:@ember/test-helpers@^2.0.0",
"ember-truth-helpers": "^3.0.0",
Expand All @@ -106,4 +106,4 @@
"typescript": "^5.4.5",
"webpack": "^5.90.3"
}
}
}
Loading