Skip to content

Consolidate Fastboot shims #172

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

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Conversation

thec0keman
Copy link
Contributor

Consolidate the import shims of fastboot into the main addon. This should fix #154

It looks as if in fastboot, both the addon/lib and public files were both being evaluated, and the addon one was taking precedence. This resulted in errors like Cannot set property compare of undefined.

I've tested this in both fastboot and the browser, both with timezone and without.

@jasonmit
Copy link
Owner

This seems reasonable and thank you for taking the time to tackle this. Any chance we can add fastboot tests? Way back when the fastboot story was non existent, unsure if that has changed recently. If it's still complicated then I'm fine with moving forward as-is.

@jasonmit jasonmit self-requested a review June 27, 2019 00:08
@thec0keman thec0keman force-pushed the fix-for-fastboot branch 3 times, most recently from f80da67 to 01a0deb Compare June 27, 2019 03:30
@thec0keman
Copy link
Contributor Author

Took a stab at some tests

@jasonmit jasonmit merged commit b06bb63 into jasonmit:master Jun 27, 2019
@iamareebjamal
Copy link

Interestingly, it was working well for us before in fastboot, but now it throws Cannot set property 'compare' of undefined after this release

@scottmessinger
Copy link

We're also running into TypeError: Cannot set property 'compare' of undefined. Not sure the cause yet.

@scottmessinger
Copy link

Appears to be an issue upgrading from 3.7.1 to 3.8.0 and running in fastboot.

@jasonmit
Copy link
Owner

jasonmit commented May 14, 2020

As far as I understand, Fastboot was broken and is now fixed except for the compare proto extension (compare was/is needed so things like Ember.computed.sort work) now missing. If none of that's true, we can probably just revert this this PR and move on.

In short, I really dislike how we've extended the proto of moment to support this Ember features.

Reason being - in the future, the compare method will no longer exist once we migrate off this addon to a ember-auto-import/webpack plugin solution - so we should think about a world where we make it work without this patch.

Perhaps there's a better solution? I don't use Ember or moment anymore in my day job (unfortunately, because I really miss it) - so it's unlikely I'll get around to patching this if the fix is beyond just reverting this commit.

Is this issue something someone else wants to take up? If so, lets open a new issue for tracking since visibility on closed PRs are difficult to find.

lan0 added a commit to lan0/ember-cli-moment-shim that referenced this pull request Apr 23, 2021
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.

Issue with new fastboot ember-engines support
4 participants