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

ember-data 4.6 fails to boot in fastboot #8101

Closed
ef4 opened this issue Aug 4, 2022 · 13 comments
Closed

ember-data 4.6 fails to boot in fastboot #8101

ef4 opened this issue Aug 4, 2022 · 13 comments

Comments

@ef4
Copy link
Contributor

ef4 commented Aug 4, 2022

This use of FastBoot.require:

const _crypto: Crypto = isFastBoot ? (FastBoot.require('crypto') as Crypto) : window.crypto;

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:

  1. Generate a new app with ember-cli 4.6.0.
  2. Install ember-data 4.6.1 and ember-cli-fastboot 3.3.2
  3. ember s
  4. Visit the homepage.

Observed failure:

Error: Unable to require module 'crypto' because it was not in the whitelist.
    at Object.require (/Users/edward/hacking/ember-data-example/node_modules/fastboot/src/fastboot-schema.js:191:15)
    at Module.callback (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/addon-tree-output/@ember-data/store/-private.js:1647:1)
    at Module.exports (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/vendor/loader/loader.js:106:1)
    at Module._reify (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/vendor/loader/loader.js:143:1)
    at Module.reify (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/vendor/loader/loader.js:130:1)
    at Module.exports (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/vendor/loader/loader.js:104:1)
    at Module._reify (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/vendor/loader/loader.js:143:1)
    at Module.reify (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/vendor/loader/loader.js:130:1)
    at Module.exports (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/vendor/loader/loader.js:104:1)
    at Module._reify (/var/folders/pj/93dgp7m57k97tby2ss0qv9nm0000gn/T/broccoli-31264hB6VoOlolJp6/out-319-broccoli_merge_trees/assets/vendor/loader/loader.js:143:1)

Workaround

Apps can add the following to package.json:

  "fastbootDependencies": [
    "crypto"
  ]
ef4 added a commit to NullVoxPopuli/embroider that referenced this issue Aug 4, 2022
I'm limiting our newest ember-data version tested to 4.4.1 because emberjs/data#8101 breaks our tests.
@runspired
Copy link
Contributor

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 crypto to their fastboot dependencies for ember-data, see

'Using createRecord in Fastboot requires you to add the "crypto" package to "fastbootDependencies" in your package.json'

            'Using createRecord in Fastboot requires you to add the "crypto" package to "fastbootDependencies" in your package.json'

Closing as this is probably something specific to embroider and this requirement has been in place for ~4 years.

@ef4
Copy link
Contributor Author

ef4 commented Aug 5, 2022

However, we have historically always enforced that users add crypto to their fastboot dependencies for ember-data

Ah, I did not know that. So what changed wasn't the requirement itself, it was the timing of when the requirement gets checked.

@runspired
Copy link
Contributor

@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 createRecord and so no uuid was ever being generated.

@mansona
Copy link
Member

mansona commented Aug 14, 2022

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 ~4.5.0, ~4.6.0, 4.7.0-beta.1, and 4.8.0-alpha.3 specifically and they all have the same issue.

Also regarding:

Closing as this is probably something specific to embroider and this requirement has been in place for ~4 years.

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.

@ef4
Copy link
Contributor Author

ef4 commented Aug 14, 2022

Yeah, I think it's probable that existing apps never happen to call createRecord in fastboot and therefore didn't notice that they're technically supposed to be allowing the crypto module.

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.

@runspired
Copy link
Contributor

@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.

@elwayman02
Copy link

@runspired where is the Crypto requirement documented? I'd love to check out that reference and learn more about it.

elwayman02 added a commit to elwayman02/ember-scroll-modifiers that referenced this issue Dec 7, 2022
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
Copy link
Contributor

@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
readme: https://github.com/emberjs/data#randomuuid-polyfill

As @jenweber and I work to build out a new docs site I'll be sure to add an SSR guide

@elwayman02
Copy link

elwayman02 commented Dec 7, 2022

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 fastbootDependencies. The only reason I found this issue is because I dug into the stack trace to find out there was a FastBoot issue coming from Ember-Data, and then I had to search this specific library (crypto) to find this issue in order to figure out how to fix it. There wasn't any information before that which would indicate what's going on to a user that isn't already intimately familiar with FastBoot.

For a developer to connect the linked randomuuid docs to the crypto library, they'd have to already know that the crypto library exists and what it does, in order to understand that it's the polyfill being used to provide that functionality in ember-data. That's a lot of leaps in assumed knowledge for something which is always required for any app using ember-data with Fastboot. :) I'm looking forward to the SSR guide on the new docs site, that should help a lot!

@runspired
Copy link
Contributor

@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

@runspired
Copy link
Contributor

@elwayman02 historically we threw this error:

getRandomValues(buffer: Uint8Array) {
try {
return (FastBoot as FastBoot).require('crypto').randomFillSync(buffer);
} catch (err) {
throw new Error(
'Using createRecord in Fastboot requires you to add the "crypto" package to "fastbootDependencies" in your package.json'
);
}
},

we could restore that, though to be honest Fastboot.require should be giving you a nice error for this, and if it isn't we should fix fastboot.

@runspired
Copy link
Contributor

@elwayman02
Copy link

elwayman02 commented Dec 7, 2022

Here's the error that shows up in the CI build output (thrown via prember in this case, I believe):

3:17:14 PM: pre-render / failed with exception: Error: Unable to require module 'crypto' because it was not in the whitelist.

When trying to reproduce locally, it throws this error and stack trace:

Error: Unable to require module 'crypto' because it was not in the whitelist.
    at Object.require (/Users/jhawker/Projects/GitHub/ember-scroll-modifiers/node_modules/fastboot/src/fastboot-schema.js:191:15)
    at Module.callback (/var/folders/wr/v2dt2mt942z423476t52cgwh0011pt/T/broccoli-69899in97ZXae598/out-484-broccoli_merge_trees/assets/addon-tree-output/@ember-data/store/-private.js:124:1)

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.

runspired added a commit to runspired/ember-cli-fastboot that referenced this issue Dec 7, 2022
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)
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

No branches or pull requests

4 participants