matrix-org / matrix-js-sdk

Matrix Client-Server SDK for JavaScript
Apache License 2.0
1.56k stars 583 forks source link

Simplify use as a git dependency in create-react-app #3015

Open davidisaaclee opened 1 year ago

davidisaaclee commented 1 year ago

Installing matrix-js-sdk as a git dependency (e.g. for a fork of the repo) in certain browser dev environments – like create-react-app – breaks builds. (I don't think this issue is limited to create-react-app, but that is what I'm using.)

Repro I can create a repo for this if it's helpful, but I'd prefer not to keep one around on my Github account.

  1. Create a new create-react-app project with TypeScript enabled, using yarn 1 as package manager: yarn create react-app matrix-js-sdk-git-dep-bug --template typescript
  2. Add matrix-js-sdk as git URL dependency: yarn add matrix-js-sdk@matrix-org/matrix-js-sdk
  3. yarn start

Project should build, but it has the following errors: https://gist.github.com/davidisaaclee/65a6d1a93b594b4d5e29fa3f352125a2

I think these are the causes:

  1. Update: Fixed in https://github.com/matrix-org/matrix-js-sdk/pull/3051 matrix-js-sdk's package.json#browser is set to ./lib/browser-index.ts, which afaict is not actually created by yarn build (it appears to be an intermediate artifact of yarn build:compile-web); so when looking for a browser entrypoint, one is not found (and so main is likely used, which causes (1) errors since src/index.ts requires transpilation but is not being transpiled, and (2) the kinds of errors that browser-index was created to solve, like undeclared global fields).
  2. matrix-js-sdk uses the prepublishOnly hook to transpile and emit type declarations. This hook is not called for dependencies specified using git URLs (https://github.com/npm/npm/issues/3055); so installing via git URL dependency never creates these necessary files.

I've solved these problems for my use case by doing the following (links to actual code changes included):

  1. Update: Fixed in https://github.com/matrix-org/matrix-js-sdk/pull/3051 Not fixed, see my comment below. Set package.json#browser to ./lib/browser-index.js, and add a package.json#types field pointing to ./lib/browser-index.d.ts. https://github.com/matrix-org/matrix-js-sdk/commit/5f9cb34a8307b64eba6d4ad5d31da855b2dd4875 https://github.com/matrix-org/matrix-js-sdk/commit/ef4f9f13c2f710dfe0ec660b4be7b2c7f1263782
    • I tried using ./dist/browser-matrix.js as browser, but with that, matrix-js-sdk's module at runtime was empty (i.e. import * as matrix from 'matrix-js-sdk' yielded Object.keys(matrix).length === 0; same for default import)
    • I see that element-web uses ./src/browser-index.ts (by way of matrix_src_browser), and passes matrix-js-sdk through Babel using its Webpack config. This isn't worth the extra configuration for my use case (especially difficult for create-react-app!); and I think the majority of people will not want to special-case matrix-js-sdk in their configs.
    • afaict, package.json#types does not have a way to export browser/Node-specific typings (a la main / browser). I am not using my fork in any Node projects, so this isn't an issue for me. I'm not sure if this would cause an issue for Node projects.
  2. Update: looks like I made a typo in the linked commit. I still think using prepack is a good approach. I'll address it if this issue gets any kind of traction. Change the prepublishOnly script to run instead at prepack (i.e. delete prepublishOnly hook in package.json, add "prepack": "yarn build") https://github.com/matrix-org/matrix-js-sdk/commit/972f256eb7b7343c13f66ec124e8a54d8b699260
    • prepare is another lifecycle hook that people use for this, but is not supported by Yarn, which I am using
    • I don't know all the places where prepack will run. It definitely runs when installing all dependencies in matrix-js-sdk (e.g. by calling yarn in the mjs project dir); that stinks, since this is unnecessary and yarn build can take some time.

I'd be happy to turn the commits above into one or two PRs, but I thought I'd write this issue first in case there are gotchas or alternatives I'm missing. (For example, I see that some of the mentioned package.json fields are being manipulated by release.sh.)

Thanks for the useful SDK!

davidisaaclee commented 1 year ago

https://github.com/matrix-org/matrix-js-sdk/pull/3051 fixes the package.json#browser entry point, but does not address prepublishOnly not running for git dependencies.

davidisaaclee commented 1 year ago

Actually, I think #3051 does not fix my use case – after pulling those changes into my fork and trying to use it as a git dependency, I get the same errors as I mentioned in the original issue description. Reverting to the solution I suggested above still works for use as git dependency.