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

Remove proces.env.EMBER_CLI_FASTBOOT #77

Merged
merged 2 commits into from
Jan 2, 2018

Conversation

kratiahuja
Copy link
Contributor

Since your addon is using process.env.EMBER_CLI_FASTBOOT for including scripts in the base page, this change is backward incompatible . You could leave your addon in the current state and it will work as is or chose to merge this and do a major version bump to support FastBoot 1.0.

Fixes the addons in ember-fastboot/ember-cli-fastboot#387

cc: @mike-north

@mike-north
Copy link
Owner

I'm not sure I understand how this would work. The included script requires parts of the DOM API that simple-dom doesn't support. Have you confirmed that fastboot builds don't throw errors or blow up?

@kratiahuja
Copy link
Contributor Author

@mike-north You are including the script in the base page (index.html). The script isn't downloaded in the fastboot environment since it is part of the base page. If it was included in vendor.js or app.js it would be eval'd.

The reason you have this check is so that you don't have the two same script tags since the beta releases of fastboot addon do double builds. With FastBoot 1.0 (soon to be released), there isn't double build and this environment flag isn't there. If you merge this PR, you will need a major version bump since your addon cannot support both versions (in this case). If you chose to keep it as is (ie not merge this PR), it will work in both versions but that check is not needed. Hence the PR.

Yes I did test it with assuming you'll do a major version bump.

@mike-north mike-north merged commit 1fa144e into mike-north:master Jan 2, 2018
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

2 participants