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

Use latest visit API #71

Merged
merged 3 commits into from Oct 16, 2015

Conversation

Projects
None yet
4 participants
@chancancode
Contributor

chancancode commented Oct 8, 2015

Update fastboot server for emberjs/ember.js#12394

Require coordination from ember-fastboot/fastboot#2

  • Take advantage of the new API
  • FastFail™
  • Don't mutate autoBoot
  • Remove FastBoot global
  • Better integration with Ember CLI
  • Move most of the work out of initializers
  • Test that it works
  • Write tests

Open question:

  • What are the rules regarding passing complex objects across the sandbox boundary? It would not be hard to avoid that if we have to.

cc @tomdale @stefanpenner

Show outdated Hide outdated index.js Outdated
Show outdated Hide outdated index.js Outdated
Show outdated Hide outdated app/initializers/fastboot.js Outdated
Show outdated Hide outdated index.js Outdated
'initializers/server/*',
'instance-initializers/server/*'
]
});
}

This comment has been minimized.

@chancancode

chancancode Oct 9, 2015

Contributor

@rwjblue @stefanpenner how does that look? with this I could complete remove the FastBoot global we inject into the sandbox.

By the way, if this works out for us, we might want to move it into the preprocessTree hook so that apps and other addons can use the same pattern

@chancancode

chancancode Oct 9, 2015

Contributor

@rwjblue @stefanpenner how does that look? with this I could complete remove the FastBoot global we inject into the sandbox.

By the way, if this works out for us, we might want to move it into the preprocessTree hook so that apps and other addons can use the same pattern

This comment has been minimized.

@rwjblue

rwjblue Oct 9, 2015

Contributor

👍 - This looks great to me!


I don't see a way to completely generalize this, but we can chat about that further in a separate issue...

@rwjblue

rwjblue Oct 9, 2015

Contributor

👍 - This looks great to me!


I don't see a way to completely generalize this, but we can chat about that further in a separate issue...

This comment has been minimized.

@chancancode

chancancode Oct 10, 2015

Contributor

Wouldn't it just work if we move this code to preprocessTreeFor? Any initializers in with a browser/... prefix will be stripped for server builds and vice versa. Maybe I am understanding it incorrectly – does preprocessTreeFor not get the fully merged app tree with user code and addon code all merged together?

@chancancode

chancancode Oct 10, 2015

Contributor

Wouldn't it just work if we move this code to preprocessTreeFor? Any initializers in with a browser/... prefix will be stripped for server builds and vice versa. Maybe I am understanding it incorrectly – does preprocessTreeFor not get the fully merged app tree with user code and addon code all merged together?

This comment has been minimized.

@rwjblue

rwjblue Oct 11, 2015

Contributor

@chancancode - Ya, we could come up with something that could work (I misread initially as postprocessTree not preprocessTree). I'm happy to come up with a specific layout/plan to make it a standard thing.

@rwjblue

rwjblue Oct 11, 2015

Contributor

@chancancode - Ya, we could come up with something that could work (I misread initially as postprocessTree not preprocessTree). I'm happy to come up with a specific layout/plan to make it a standard thing.

This comment has been minimized.

@chancancode

chancancode Oct 12, 2015

Contributor

I'll leave that for another time, probably until someone asks for it :P (if you are reading this, you might just want to PR it 😉)

@chancancode

chancancode Oct 12, 2015

Contributor

I'll leave that for another time, probably until someone asks for it :P (if you are reading this, you might just want to PR it 😉)

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Oct 12, 2015

Contributor

Got the existing tests passing today with some minor fixes! I'll clean it up tomorrow!

Contributor

chancancode commented Oct 12, 2015

Got the existing tests passing today with some minor fixes! I'll clean it up tomorrow!

Show outdated Hide outdated index.js Outdated
},
APP: {
autoboot: false
}

This comment has been minimized.

@chancancode

chancancode Oct 12, 2015

Contributor

both of these works

@chancancode

chancancode Oct 12, 2015

Contributor

both of these works

Show outdated Hide outdated package.json Outdated
Show outdated Hide outdated tests/acceptance/simple-test.js Outdated
Use latest visit API from 2.2 beta
Most of the heavy lifting (runtime stuff) has been moved to
`ember-fastboot-server`. This addon is currently now only responsbile for
building this right app bundle for the fastboot server to consume.

The main integration point between the fastboot server and the app
bundle is the injected `~fastboot/app-factory` module. This allows the
server to create an `Application` instance (not to be confused with an
`ApplicationInstance` instance ...:trollface:) without having to
discover the app name and the app config.

This commit also introduced a convention within the addon: all
(instance) initializers within the `browser` directory will be excluded
from the fastboot app bundle; conversely, the (instance) initializers
contained within the `server` bundle will be removed from the regular
build.

chancancode added a commit to chancancode/ember-fastboot-server that referenced this pull request Oct 14, 2015

Use latest visit API from 2.2 beta
This commit has an implicit cross-dependency on ember-fastboot/ember-cli-fastboot#71.

This commit takes advantage of the new visit API in Ember and moves
most of the heavy-lifting (runtime stuff) into the server.

@chancancode chancancode changed the title from [WIP] Use latest visit API to Use latest visit API Oct 14, 2015

chancancode added some commits Oct 14, 2015

Server 'normal' browser build with --serve-assets
We are producing different content for the server vs browser app bundle
(e.g. the server one has `autoRun` and `autoboot` disabled), so we need
to serve the browser version with `--serve-assets`, otherwise the app
won't boot in the browser.

The `--assets-path` is somewhat of a quick fix. We should probably run
the build process twice and store them in the same location but with a
different suffix, like `dummy.fastboot.js` and `index.fastboot.html`.
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Oct 14, 2015

Contributor

Tests are green, fixed all the problems, backfilled the commit messages, did some smoke tests 😬

@tomdale @rwjblue final review?

If this looks okay to you, we should probably first merge ember-fastboot/fastboot#2, release 0.1.0, then update the last commit to use the released version before merging.

Contributor

chancancode commented Oct 14, 2015

Tests are green, fixed all the problems, backfilled the commit messages, did some smoke tests 😬

@tomdale @rwjblue final review?

If this looks okay to you, we should probably first merge ember-fastboot/fastboot#2, release 0.1.0, then update the last commit to use the released version before merging.

@@ -36,7 +37,10 @@ module.exports = function runServer(callback, options) {
args.push(commandOptions);
return new RSVP.Promise(function(resolve, reject) {
runCommand.apply(null, args)
runCommand.call(null, emberCommand, 'build') // build 'dist'

This comment has been minimized.

@chancancode

chancancode Oct 14, 2015

Contributor

So this reflects a real-world problem: you need to now run ember build (probably ember build -prod) before you run ember fastboot --serve-assets, because we need to serve the browser builds from dist. (See the commit message for details)

Unfortunately, it is not easy to refactor the set up to produce both builds in a single command (it relies on a global ENV variable, among other things).

So I just added a new assets-path option for now, but we should revisit this in the future.

@chancancode

chancancode Oct 14, 2015

Contributor

So this reflects a real-world problem: you need to now run ember build (probably ember build -prod) before you run ember fastboot --serve-assets, because we need to serve the browser builds from dist. (See the commit message for details)

Unfortunately, it is not easy to refactor the set up to produce both builds in a single command (it relies on a global ENV variable, among other things).

So I just added a new assets-path option for now, but we should revisit this in the future.

@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale Oct 16, 2015

Contributor

Released 0.1.0 of ember-fastboot-server, I will merge this and update the dependency.

Contributor

tomdale commented Oct 16, 2015

Released 0.1.0 of ember-fastboot-server, I will merge this and update the dependency.

tomdale added a commit that referenced this pull request Oct 16, 2015

@tomdale tomdale merged commit 9573c31 into ember-fastboot:master Oct 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

arjansingh pushed a commit to arjansingh/ember-cli-fastboot that referenced this pull request Oct 29, 2016

Merge pull request ember-fastboot#71 from jasonmit/patch-1
Only access instance.getURL if the instance has booted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment