-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
ember-data 4.6 fails to boot in fastboot #8101
Comments
I'm limiting our newest ember-data version tested to 4.4.1 because emberjs/data#8101 breaks our tests.
We have to revert this change anyway because the browser's uuid generation is currently gated behind secure contexts, though our decision to revert also appears to have helped motivate browser reps to revisit their decision to do so. See #8097 However, we have historically always enforced that users add
Closing as this is probably something specific to embroider and this requirement has been in place for ~4 years. |
Ah, I did not know that. So what changed wasn't the requirement itself, it was the timing of when the requirement gets checked. |
@ef4 we wouldn't have done the require check in the past until a uuid was actually generated, with the lack of need of polyfill (in theory, obviously turns out to be problematic given secure context restriction) we moved to eagerly determining the crypto module. More than likely nothing in your fastboot setup was calling |
Great that this has been fixed 🎉 Is there a plan to backport it and release it as a patch release for v4.6.x? Edit: I just checked a bunch of versions (trying to find one that fixes a deprecation) and it looks like this is present in the v4.5 series all the way up to the v4.8 series. I tested Also regarding:
I'm seeing this problem with ember-source@latest and not using embroider so this is a real problem that people can see in the wild. |
Yeah, I think it's probable that existing apps never happen to call In my case, I noticed this in a test that uses ember-data in a read-only capacity, which seems not unrealistic for some apps. |
@mansona the thing that has been fixed is randomUUID sometimes needing a polyfill. Crypto has always been required and should be added to the fastbootDependencies. While today we only use it for createRecord that's unlikely to stay the case for much longer, especially once we begin serializing the cache for rehydration. |
@runspired where is the Crypto requirement documented? I'd love to check out that reference and learn more about it. |
Ember-Data has an opaque dependency on the `crypto` package, which causes FastBoot to fail when booting up the field-guide documentation app. This adds `crypto` to `fastbootDependencies` to allow our docs to boot properly. More info: emberjs/data#8101
@runspired it was documented by the error itself which would tell you what was needed We should probably add the fastboot note here where we document the polyfill though: api docs: https://api.emberjs.com/ember-data/release/modules/ember-data-overview?show=inherited As @jenweber and I work to build out a new docs site I'll be sure to add an SSR guide |
FWIW, the error itself isn't really clear as to what's going on, why it's happening, or how to fix it. The error message doesn't even say it has anything to do with FastBoot - you have to dig into the stack trace to figure that out, and even then it's not clear that it's something you can fix as easily as adding to For a developer to connect the linked randomuuid docs to the |
@elwayman02 it is possible we have lost an error message we used to have, it used to be extremely explicit that adding the crypto package to fastbootDependencies was what was needed |
@elwayman02 historically we threw this error: data/packages/store/addon/-private/identifiers/utils/uuid-v4.ts Lines 11 to 19 in 3122d68
we could restore that, though to be honest |
@elwayman02 you should have gotten this (meaningful) error https://github.com/ember-fastboot/ember-cli-fastboot/blob/703e78832fbb8d4c9d9ace9d9aee6a4e7a72bed5/packages/fastboot/src/fastboot-schema.js#L197-L203 |
Here's the error that shows up in the CI build output (thrown via
When trying to reproduce locally, it throws this error and stack trace:
That's the only information it gives you to go off of. The text "because it was not in the whitelist" doesn't really have any meaning without context, which meant having to dig several layers further to actually understand what was going on. Even the fastboot code itself doesn't clearly document what the whitelist is or where it comes from. |
In emberjs/data#8101 it was noted that the error message provided here did not give enough context for users to understand the issue was due to the Fastboot environment and the existence of the fastbootDependencies allow list. This should help greatly with that. Ref: emberjs/data#8101 (comment)
This use of
FastBoot.require
:data/packages/store/addon/-private/identifier-cache.ts
Line 34 in 637a7d4
is only legal if the app explicitly adds
"crypto"
to the fastboot whitelist, which is not the default. Out of the box, this will cause a crash at boot.Reproduction:
ember s
Observed failure:
Workaround
Apps can add the following to package.json:
The text was updated successfully, but these errors were encountered: