Skip to content

Throw a helpful error when people use isFastboot instead of isFastBoot#814

Merged
rwjblue merged 1 commit intoember-fastboot:masterfrom
bertdeblock:throw-when-using-nonexistent-fastboot-service-property
Mar 18, 2021
Merged

Throw a helpful error when people use isFastboot instead of isFastBoot#814
rwjblue merged 1 commit intoember-fastboot:masterfrom
bertdeblock:throw-when-using-nonexistent-fastboot-service-property

Conversation

@bertdeblock
Copy link
Contributor

headers: deprecatingAlias('request.headers', { id: 'fastboot.headers-to-request', until: '0.9.9' }),
isFastBoot: typeof FastBoot !== 'undefined',

isFastboot: computed(function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or should we warn here instead and return this.isFastBoot?

Copy link
Member

Choose a reason for hiding this comment

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

I think an assertion is fine, though I'd prefer if we didn't make this a computed (it will only assert the first time it is accessed I think).

We should consider migrating to native classes here, and then using a native getter directly. Unfortunately, we'll have to do this conditionally based on Ember version. We need to drop Ember < 3.12 support, would you mind making an issue for discussing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #815 for this. Should I just leave the PR until using a native getter is possible or?

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

Woop thanks for the contribution 🎉

@rwjblue rwjblue merged commit 2a6ed5d into ember-fastboot:master Mar 18, 2021
@bertdeblock bertdeblock deleted the throw-when-using-nonexistent-fastboot-service-property branch September 1, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants