search

mitchlloyd / ember-screen

A screen size service for Ember
MIT License
50 stars 10 forks source link

Add FastBoot support #3

Closed simonihmig closed 7 years ago

simonihmig commented 7 years ago

This PR adds support for FastBoot:

simonihmig commented 7 years ago

@mitchlloyd anything missing here?

simonihmig commented 7 years ago

@mitchlloyd Thanks for the review and for inviting me. Will make the requested changes over the next 1-2 days...

simonihmig commented 7 years ago

@mitchlloyd Just pushed the requested changes, hope this is ok!

Btw, forgot to mention that testing for FastBoot is done using the ember fastboot:test command (the testing addon brings that). Though I am unsure how to integrate that into CI. Have no idea about codeship...

mitchlloyd commented 7 years ago

Thanks for making these changes. I want to try a little refactoring on the window classes and I'll see if I can get that testing task into CI.

mitchlloyd commented 7 years ago

Hey @simonihmig, I pinged you on Ember Slack, but wanted to follow up here too. Whenever I run ember fastboot:test, the process seems to just hang with this message:

Running Fastboot tests! This may take a while...

  index

I tried some initial debugging but didn't get far. Do you have any hunches about what might be wrong or something I can do to debug?

mitchlloyd commented 7 years ago

Ok, I accidentally left this process running, after trying it again and it takes 1m16s to run the 2 tests on my machine. Is this expected? Is there something wrong with my setup? I'm using Node version v6.9.2.

simonihmig commented 7 years ago

Yes, it is actually expected that the tests take some minutes. This is because a temporary app is created (ember new) including npm install, based on ember-cli-addon-tests, to run the addon with FastBoot in the context of an app.

mitchlloyd commented 7 years ago

I made a few changes and pushed up a local branch so I could work on the CI integration here: https://github.com/mitchlloyd/ember-screen/tree/simonihmig-fix-fastboot. Could you checkout the changes I made and see if any seem ill-advised? In particular I used the global FastBoot short cut that I saw other Ember addons use.

If everything looks good to go, you can either open up your fork branch for "allow edits from maintainer" or I can use the other branch to merge. I'd like to squash to 2 commits: one for the ember-cli upgrade and then one for this feature.

simonihmig commented 7 years ago

@mitchlloyd LGTM. Added just some comments to your commits. Already had the "allow edits from maintainer" flag enabled, feel free to merge/squash!

mitchlloyd commented 7 years ago

Yeah in particular let me know if using the FastBoot global is a big no-no. From this issue I saw that ember-ajax and another addon were taking this approach. I'm hoping I'll be safe in the pack :)

mitchlloyd commented 7 years ago

I've realized that installing ember-cli-fastboot as a dev dependency and running ember fastboot renders the app in a couple seconds as opposed to a couple minutes with this PR on my machine. For an app that doesn't do anything special at install time, is there a reason to go through this install process?

I wonder is just booting the dummy app with ember fastboot, visiting the URL and asserting on the DOM would work better for this use case.

simonihmig commented 7 years ago

I wonder is just booting the dummy app with ember fastboot, visiting the URL and asserting on the DOM would work better for this use case.

Would work as well, but a) you would have to write all the test harness yourself, as ember-fastboot-addon-tests does not support that b) you cannot use the fixtures to create a customized testing app, would all have to go into the default dummy app c) you cannot test with different apps. We could for example add tests using a different testing-app (different set of fixtures), so we could test that it is possible to override the defaults you introduced to NullWindow in say an instance-initializer.

simonihmig commented 7 years ago

Using the FastBoot global seems to be ok. Probably makes sense for addons, where we cannot just use Ember.inject.service() to get the FastBoot service (as the consuming app might not use FastBoot and injecting a non-existing service will throw an exception). That's why the initial implementation required some more work, but indeed not needed when going the global-way...

mitchlloyd commented 7 years ago

Merged this PR as 17f9905ca3436b181f53c18fd9ada82ae223abcc and 4ee1ad4e17d55d7256a23da53e3612acb5f694f4. Will release as 0.2.0.

simonihmig commented 7 years ago

Nice! 👍