Skip to content

Refactor instance-initializer#45

Merged
rwjblue merged 3 commits intoember-fastboot:masterfrom
mydea:engines
Jan 3, 2018
Merged

Refactor instance-initializer#45
rwjblue merged 3 commits intoember-fastboot:masterfrom
mydea:engines

Conversation

@mydea
Copy link
Copy Markdown

@mydea mydea commented Jan 3, 2018

This PR does two main things:

Basically, in lazy loading engines, the instance initializer will run whenever another engine is entered, thus trashing the head.

This PR changes this, so that the head is trashed in the init() hook of the head-layout component.
In addition to fixing this issue, it should also make it much easier to overwrite/adapt this behavior for special requirements.

@mydea mydea changed the title Engines Refactor instance-initializer Jan 3, 2018
@mydea mydea force-pushed the engines branch 2 times, most recently from e9ee853 to 6f2178a Compare January 3, 2018 13:42
Copy link
Copy Markdown
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Looks good, I like the approach 👍 .

Thanks for working on this!

document.head.removeChild(startMeta);
document.head.removeChild(endMeta);
}
// do nothing!
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you move this comment directly down into the initialize method below, and remove this extra function?

@mydea mydea force-pushed the engines branch 2 times, most recently from be072ef to fc638a9 Compare January 3, 2018 14:19
@mydea
Copy link
Copy Markdown
Author

mydea commented Jan 3, 2018

I removed the extra function in the instance initializer and moved the comments down!

Comment thread addon/components/head-layout.js Outdated
get,
computed,
getOwner
} = Ember;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, since this is moved to addon/** we should start migrating to the newer style imports directly.

This would become:

import Component from '@ember/component';
import { get } from '@ember/object';
import { computed } from '@ember/object';
import { getOwner } from '@ember/application';

Comment thread addon/components/head-layout.js Outdated
@@ -0,0 +1,63 @@
import Ember from 'ember';
import layout from 'ember-cli-head/templates/components/head-layout';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd generally prefer relative imports here, but I don't feel super strongly...

This would become:

import layout from '../templates/components/head-layout';

Francesco Novy added 2 commits January 3, 2018 15:37
Previously, any eventual head data built by fastboot was cleaned up in an instance initializer. This was now moved into the head-layout component, where this happens on init.

This fixes an issue where this cleanup happens multiple times when using lazy loading engines.

Additionally, this moves the head-layout component into the addon namespace, which makes it possible to overwrite/extend from it.
@mydea
Copy link
Copy Markdown
Author

mydea commented Jan 3, 2018

I adjusted the import paths for the head-layout component according to your suggestions :)

@rwjblue rwjblue merged commit 7f31be0 into ember-fastboot:master Jan 3, 2018
@mydea
Copy link
Copy Markdown
Author

mydea commented Feb 14, 2018

Could we get a new release with that fix? That would be great!

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented May 8, 2018

So sorry @mydea, I completely lost track of this! It has been released as 0.4.1...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make FastBoot DOM smashing opt-in

2 participants