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.

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

Unify instance initializers. #31

Closed

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Jun 5, 2017

In fastboot 1.0 the concept of environment-exclusive initializers is gone. Instead,
addons are encouraged to use the existence of the Fastboot constant to know about
the environment the app is running on.

This PR unifies the fastboot/head.js initializers into the main one

In fastboot 1.0 the concept of environment-exclusive initializers is gone. Instead,
addons are encouraged to use the existence of the `Fastboot` constant to know about
the environment the app is running on.

This PR unifies the fastboot/head.js initializers into the main one
component.appendTo(document.head);
const component = owner.lookup('component:head-layout');
component.appendTo(document.head);
} else {
Copy link

@kratiahuja kratiahuja Jun 5, 2017

Choose a reason for hiding this comment

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

You are merging the fastboot initializer here and sending the extra bytes to browser that will never be run. Ideally you can move the new existing app/instance-initializer/fastboot/head.js to fastboot/instance-initializers/head.js. That's how we envisioned apps moving the fastboot initializers. Also currently the rc builds auto migrate the fastboot initializers to their new location with throwing a warning asking addons to the do the same.

I would suggest you to take that approach and then this addon does a major version bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I misunderstood the instructions then. I'll update it.

Choose a reason for hiding this comment

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

I'm really confused on what the suggested way to handle fastboot/browser code is. Originally it was separate builds, then it was a single build with flags guarding, now it seems there will be separate builds again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From slack conversation, initializers in /app/(instance-)initializers will run both in fastboot and the browser, so you have to guard if they do fastboot-incompatible stuff.

Initializers in /fastboot/(instance-)initializers will only run in fastboot so there is no need to guard.

Choose a reason for hiding this comment

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

@bcardarella There's one build that runs in both browser & node. There can be an additional file(s) that are loaded only in fastboot. The files that are loaded only in fastboot are loaded by adding the file to the fastboot manifest. For more, check out: ember-fastboot/ember-cli-fastboot#413

Copy link

@kratiahuja kratiahuja Jun 5, 2017

Choose a reason for hiding this comment

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

I'm really confused on what the suggested way to handle fastboot/browser code is. Originally it was separate builds, then it was a single build with flags guarding, now it seems there will be separate builds again?

There are still no separate builds now. We are building the additional asset containing such fastboot overrides. For the fastboot initializers we never asked to guard against the flags. It's only for the browser initializers that you need to move from app/initializers/browser/somefile.js to app/initializers/somefile.js and put the guard. For the app/initializers/fastboot/somefile.js you can keep them for now to be backward compatible in both worlds and when your addon only decides to support FastBoot 1.0 , move them to fastboot/initializers/somefile.js.

We are working on updating the fastboot website to talk about the new scheme and new structure. There is also a cheatsheet that talks about the various migration cases.

Choose a reason for hiding this comment

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

@kratiahuja I might be misunderstanding things, but by adding a file to the fastboot manifest, aren't you creating a file that is loaded only in fastboot?

Choose a reason for hiding this comment

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

aren't you creating a file that is loaded only in fastboot

this is my understanding as well, which would definitely seem like a separate build. Outside the scope of discussion for this repo but the churn on Fastboot 1.0 is very difficult to keep up on.

Copy link

@kratiahuja kratiahuja Jun 5, 2017

Choose a reason for hiding this comment

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

I might be misunderstanding things, but by adding a file to the fastboot manifest, aren't you creating a file that is loaded only in fastboot?

@scottmessinger yes. The usecase that you are talking about earlier is if you have to import external libraries in fastboot environment. The usecase in this PR is different than that are refers to case 4 in the cheatsheet which is auto fixed for addons using the migration.

Outside the scope of discussion for this repo but the churn on Fastboot 1.0 is very difficult to keep up on.

@bcardarella Have you seen the migration cheatsheet? Would love your feedback on the issue Scott posted with any additional usecases that seem confusing for you and we can work with making things better. Any help to make it better is greatly appreciated.

Things will be much simpler to understand once the website is updated.

@cibernox
Copy link
Contributor Author

cibernox commented Jun 5, 2017

I close this, superseded by #32

@cibernox cibernox closed this Jun 5, 2017
@cibernox cibernox deleted the unify-instance-initializers branch June 5, 2017 21:00
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.

4 participants