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

Fastboot Safe #348

Merged
merged 2 commits into from Jan 6, 2018
Merged

Fastboot Safe #348

merged 2 commits into from Jan 6, 2018

Conversation

demersus
Copy link
Contributor

Our fastboot builds were failing for local development. This prevents the addon from enabling itself in the fastboot build. (Addon depends on jquery)

@demersus demersus changed the title Check for EMBER_CLI_FASTBOOT in env before including files Fastboot Safe Dec 13, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.063% when pulling d51e93a on demersus:feature/fastboot-safe into 6a65ceb on san650:master.

index.js Outdated
@@ -49,7 +49,12 @@ module.exports = {
// TODO: In order to make the addon work in EmberTwiddle, we cannot use // the `tests` prop til
// https://github.com/joostdevries/twiddle-backend/pull/28 is merged.
// return !!this.app.tests;
return this.app.env !== 'production';

if('EMBER_CLI_FASTBOOT' in process.env) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would fail when:

EMBER_CLI_FASTBOOT=false ember s

can we change this condition to check if the value is true, something like process.env.EMBER_CLI_FASTBOOT === 'true'?

@ro0gr
Copy link
Collaborator

ro0gr commented Dec 13, 2017

@demersus I don't have experience with fastboot but I'm curious about EMBER_CLI_FASTBOOT variable. Do you set it manually or is it fastboot built-in feature?

@san650
Copy link
Owner

san650 commented Dec 13, 2017

Thanks @demersus for the PR.

As @ro0gr suggested, can you update the code to check that the value is 'true'?

process.env.EMBER_CLI_FASTBOOT === 'true';

@ro0gr doing a quick search in github, it seems that this VAR is what people uses to check this.

About the failings in the CI, they don't seem related with this PR, it seems that ember-data breaks our test suite 😞

@demersus
Copy link
Contributor Author

Yes, EMBER_CLI_FASTBOOT is present when this is building/running in fastboot. I'll update the check to check for the correct value. No idea what went wrong in the CI tests

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.063% when pulling 89c4a0c on demersus:feature/fastboot-safe into 6a65ceb on san650:master.

@demersus
Copy link
Contributor Author

As for the process.env.EMBER_CLI_FASTBOOT treated as a boolean itself: I've seen this pattern used in other projects on Github. It seems to work in my test app fine.

@ro0gr
Copy link
Collaborator

ro0gr commented Jan 2, 2018

@san650 I've re-ran failing builds. The issue seems to be fixed now. Is this good enough to be merged?

@san650 san650 merged commit 3689d8e into san650:master Jan 6, 2018
@san650
Copy link
Owner

san650 commented Jan 6, 2018

Thanks @demersus for contributing!

@bobisjan
Copy link

bobisjan commented Jan 6, 2018

Hi,

I can tell that EMBER_CLI_FASTBOOT flag is not available in FastBoot 1.0+, ember-fastboot/ember-cli-fastboot#369, ember-fastboot/ember-cli-fastboot#399 and thus should not be used any more.

In our application we have both stable ember-cli-fastboot and ember-cli-page-object without any problems for some time.

I've tested ember-cli-page-object@1.13.0-alpha.1 within our application and can confirm that it's failing to serve in FastBoot due to:

TypeError: Cannot read property 'noConflict' of undefined
    at /.../tmp/broccoli_persistent_filtersimple_replace-output_path-xKFvdG3g.tmp/assets/vendor/shims/ecpo-jquery-global.js:7:1
    at /.../tmp/broccoli_persistent_filtersimple_replace-output_path-xKFvdG3g.tmp/assets/vendor/shims/ecpo-jquery-global.js:9:1
    at ContextifyScript.Script.runInContext (vm.js:35:29)

But, I've also tested ember-cli-page-object@master within our application and can confirm that it's serving in FastBoot without any problem 🎉.

@san650
Copy link
Owner

san650 commented Jan 6, 2018

@bobisjan thanks for the heads up.

Still not clear to me if we need to change our implementation to avoid including the addon on a fastboot build. When you say master is working, are you referring to master with this PR merged?

@bobisjan
Copy link

bobisjan commented Jan 6, 2018

The error I've mentioned was fixed in master by #346, not by this PR.

I don't know which version of ember-cli-page-object @demersus was using, but I can imagine that #346 also fixed his problem.

In FastBoot 1.0+ addon's code can not be actually excluded, but it can be wrapped with if statement to prevent from being evaluated, https://github.com/ember-fastboot/ember-cli-fastboot#build-hooks-for-fastboot.

I don't think that it is required to change implementation (because it's working), but it would be useful to have some basic test to prevent from regression using https://github.com/tomdale/ember-cli-addon-tests.

@san650 san650 mentioned this pull request Jan 6, 2018
@san650
Copy link
Owner

san650 commented Jan 6, 2018

Thanks @bobisjan for the explanation. I've created the an issue to create a test for fastboot. See #362

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.

None yet

5 participants