phosphorjs / phosphor

The PhosphorJS Library
BSD 3-Clause "New" or "Revised" License
1.04k stars 168 forks source link

coreutils: Missing dependency "crypto" #353

Open GordonSmith opened 6 years ago

GordonSmith commented 6 years ago

random.ts has a "require" for "crypto", but package.json has no reference to that package: https://github.com/phosphorjs/phosphor/blob/51a653011dae5533dcc3097e193b70b32ff9fe24/packages/coreutils/src/random.ts#L42 https://github.com/phosphorjs/phosphor/blob/39d54e8a5863f4e96b80dc05404e07367af5e638/packages/coreutils/package.json#L14

Edit: After a bit of digging I see that "crypto" is now a core Node module and that while you do have runtime checking for the current environment (Node or Browser), it hasn't been isolated for downstream bundlers in general.

FWIW in scenarios like this I typically create two "index.ts" files:

Then update package.json with:

  "main": "dist/index.server.js",
  "browser": "dist/index.client.js",

I can issue a PR for the above if you like (but want to make sure you are happy with the proposed solution before I do the work).

blink1073 commented 6 years ago

Hi @GordonSmith, see discussion in https://github.com/phosphorjs/phosphor/pull/349.

GordonSmith commented 6 years ago

@blink1073 That PR works 100% for me (and all browser users) - but if there is a genuine need to fallback to the Node crypto library for some reason (the coreutils might be used in other scenarios like unit tests etc.), then my proposed solution might be more acceptable to @sccolbert

(thx for the cross-link BTW)

blink1073 commented 6 years ago

Does webpack honor the browser field for the entry point?

GordonSmith commented 6 years ago

Yes (99% sure) - webpack defaults to browser = true as well (which shouldn't be a surprise).

SimonBiggs commented 6 years ago

@gordonsmith my method was a hacky workaround to be sure. We should close my pull request and replace it with yours.