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

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

Open
davidisaaclee opened this issue Jan 2, 2023 · 2 comments
Open

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

davidisaaclee opened this issue Jan 2, 2023 · 2 comments

Comments

@davidisaaclee
Copy link
Contributor

davidisaaclee commented Jan 2, 2023

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 Fix browser entrypoint #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 (run prepublish for git url packages npm/npm#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 Fix browser entrypoint #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. 5f9cb34 ef4f9f1
  • 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.
  1. 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") 972f256
  • 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
Copy link
Contributor Author

#3051 fixes the package.json#browser entry point, but does not address prepublishOnly not running for git dependencies.

@davidisaaclee
Copy link
Contributor Author

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.

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

1 participant