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

Ensure style compilation works properly with ember-cli >= 3.18 #934

Merged
merged 4 commits into from
Aug 27, 2021
Merged

Ensure style compilation works properly with ember-cli >= 3.18 #934

merged 4 commits into from
Aug 27, 2021

Conversation

stefanpenner
Copy link
Collaborator

@stefanpenner stefanpenner commented Aug 23, 2021 •
edited
Loading

Bump test-packages/support’s to known failing version of ember-cli for debugging.

I'm using this PR to help narrow down specific CI failures across platforms.

(context: this is an exploratory fix for -> #927)

[fixes #927]

@stefanpenner stefanpenner marked this pull request as draft August 23, 2021 21:52
@stefanpenner stefanpenner changed the title Bump test-packages/support’s to known failing version of ember-cli fo… Bump test-packages/support’s to known failing version of ember-cli for debugging Aug 23, 2021
@stefanpenner stefanpenner force-pushed the compiled-styles-issue branch from 70da86c to e01cc59 Compare August 24, 2021 22:41
@eoneill eoneill mentioned this pull request Aug 25, 2021
Merged
@ef4
Copy link
Contributor

ef4 commented Aug 25, 2021

These tests haven't been ported to scenario-tester yet, which is why we didn't see this sooner.

@stefanpenner
Copy link
Collaborator Author

stefanpenner commented Aug 25, 2021 •
edited
Loading

@ef4 ya porting to scenario-tester seems critical. My prioritization for this PR though is: 1) land a fix 2) migrate tests scenario-tester as it seems like (1) basically blocks (2). WDYT?

I'll sink up with @thoov to understand the scenario-tester migration stuff better, and attempt to lend a hand.

@stefanpenner stefanpenner force-pushed the compiled-styles-issue branch from e01cc59 to 44178da Compare August 27, 2021 03:24
@stefanpenner
[BUGFIX] Handle ember-cli >= 3.18’s __COMPILED_STYLES__ dir layout ch…
13b587d 
…ange

Ember 3.18 moves add-on styles into `<addon-name>/__COMPILED_STYLES`, this isn’t compatible with embroider, so we move it back…
@stefanpenner stefanpenner force-pushed the compiled-styles-issue branch from 44178da to 13b587d Compare August 27, 2021 04:11
@stefanpenner stefanpenner changed the title Bump test-packages/support’s to known failing version of ember-cli for debugging handle ember-cli >= 3.18's __COMPILED_STYLES dir change Aug 27, 2021
@stefanpenner stefanpenner added the bug Something isn't working label Aug 27, 2021
stefanpenner
stefanpenner commented Aug 27, 2021
View reviewed changes
packages/compat/src/v1-app.ts
@@ -97,6 +99,11 @@ export default class V1App {
return dirname(emberCLIPackage);
}

@Memoize()
get hasCompiledStyles() {
return semver.gte(JSON.parse(readFileSync(`${this.emberCLILocation}/package.json`, 'utf8')).version, '3.18.0');
Copy link
Collaborator Author

@stefanpenner stefanpenner Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@rwjblue I'm open to suggestions to detecting this in a different way...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ya this seems good to me, especially since we already have the this.emberCLILocation setup

@stefanpenner stefanpenner marked this pull request as ready for review August 27, 2021 04:17
stefanpenner
stefanpenner commented Aug 27, 2021
View reviewed changes
packages/compat/src/v1-addon.ts
@@ -738,6 +738,21 @@ export default class V1Addon {
private buildAddonStyles(built: IntermediateBuild) {
let addonStylesTree = this.addonStylesTree();
if (addonStylesTree) {
if (this.app.hasCompiledStyles) {
Copy link
Collaborator Author

@stefanpenner stefanpenner Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

rather then detected this for each add-on, i memoize the result on the app instance and reuse said result for each addon's addon styles

stefanpenner
stefanpenner commented Aug 27, 2021
View reviewed changes
test-packages/support/package.json
@@ -14,7 +14,7 @@
"broccoli": "^3.4.2",
"console-ui": "^3.0.0",
"ember-auto-import": "^1.2.21",
"ember-cli": "~3.17.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once this is green, I would like to test bumping this to latest. I have chosen the most conservative bump to minimize variables as I resolve the issue at hand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've bumped to latest and everything remains green

@stefanpenner stefanpenner mentioned this pull request Aug 27, 2021
Closed
@stefanpenner
Copy link
Collaborator Author

stefanpenner commented Aug 27, 2021 •
edited
Loading

Now that this is green, let me try to bump all the ember-cli to latest. If that doesn't work, i'll roll that change back so we can land this.

@stefanpenner @rwjblue
Update packages/compat/src/v1-addon.ts
a04ee3f 
Co-authored-by: Robert Jackson <rjackson@linkedin.com>
@rwjblue rwjblue merged commit 7a825df into master Aug 27, 2021
@rwjblue rwjblue deleted the compiled-styles-issue branch August 27, 2021 19:43
@rwjblue rwjblue changed the title handle ember-cli >= 3.18's __COMPILED_STYLES dir change Ensure style compilation works properly with ember-cli >= 3.18 Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test Failures suggest potential incompatibilities with ember-cli 3.18+
3 participants
@stefanpenner @ef4 @rwjblue