search

anza-xyz / wallet-adapter

Modular TypeScript wallet adapters and components for Solana applications.
https://anza-xyz.github.io/wallet-adapter/
Apache License 2.0
1.59k stars 960 forks source link

Create React App is broken by lack of default polyfills in Webpack 5 #241

Closed cogoo closed 2 years ago

cogoo commented 2 years ago

Edit by @jordansexton:

To anyone looking for a very good workaround right now, don't use Create React App. Just use the react-ui-starter or material-ui-starter projects with Parcel, or the nextjs-starter project with Next.js, save yourself a huge hassle, and get much faster builds with SWC instead of Babel.

Describe the bug Broken build with CRA v5, unable to resolve modules.

To Reproduce Steps to reproduce the behavior:

  1. Initialize a new CRA project. npx create-react-app my-app-test
  2. Install and use any wallet-adapter packages
  3. See error
BREAKING CHANGE: The request './adapter' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.

Solution Relative imports must include a file extension to follow ESM strict mode requirements introduced in Webpack 5. It's valid typescript to include .js extensions as part of the import path 🤯 :

example.ts

import { Example } from './example.js';

compiles to

example.js

import { Example } from './example.js';
jordaaash commented 2 years ago

It's worth noting that yarn lint is also currently broken.

jordaaash commented 2 years ago

I discovered this when I started working on adding check for import extensions to eslint: https://github.com/solana-labs/wallet-adapter/commit/7b443655c9b8c196fa952a9cf904ba1e200f3e9d

cogoo commented 2 years ago

Recommended solution seems to be:

You must use a .js extension in relative imports even though you're importing .ts files.

Full breakdown: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

cogoo commented 2 years ago

It's worth noting that yarn lint is also currently broken.

🧐 this is unrelated. Fixed this by adding the missing transient dependencies

yarn add eslint-config-react-app babel-eslint -W --dev

or by removing react-app/jest from material-ui-starter/package.json and react-ui-starter/package.json

 "eslintConfig": {
        "extends": [
             "react-app",
-            "react-app/jest"
        ]
    },
Eliascm17 commented 2 years ago

I'm experiencing this in a CRA v5 app as well.

image

Screen Shot 2022-01-05 at 11 42 09 AM
Malai237 commented 2 years ago

I am facing the same issues and I am using node

nicechute commented 2 years ago

Need to fix this asap

jordaaash commented 2 years ago

Linting and the starter projects are fixed thanks to @cogoo in #245.

This issue is otherwise blocked until Craco fully supports CRA 5 / Webpack 5 and we can configure Webpack to build with ESM modules that don't use .mjs extensions.

Until then, please use one of the starter projects that use Next.js or CRA 4. They work right now and don't require additional configuration.

Instructions to run the React UI + CRA4 starter project are here: https://github.com/solana-labs/wallet-adapter#build-from-source

kevinforrestconnors commented 2 years ago

Any recommendation to those who have already started a project? I'd rather not remake my project and just want to fix this to continue working

jordaaash commented 2 years ago

Nope, there's no good workaround at this time, sorry. CRA 5 is not supported.

jordaaash commented 2 years ago

It's relatively straightforward to take your code from your CRA 5 app and drop it into the CRA 4 based react-ui-starter project though.

kevinforrestconnors commented 2 years ago

fair enough, thanks

Malai237 commented 2 years ago

Hey guys, I got it to work. I uninstalled the libraries listed below and downloaded them again with these specific versions: "@solana/wallet-adapter-base": "^0.7.1", "@solana/wallet-adapter-react": "^0.13.1", "@solana/wallet-adapter-react-ui": "^0.6.1", "@solana/wallet-adapter-wallets": "^0.11.3", "@solana/web3.js": "^1.31.0", ... "react-scripts": "4.0.3",

jordaaash commented 2 years ago

@Malai237 you don't need to downgrade the packages to get this to work with CRA 4. The react-ui-starter project works out of the box with the latest versions of the packages using CRA 4.

jordaaash commented 2 years ago

Just published:

kevinforrestconnors commented 2 years ago

Instructions to run the React UI + CRA4 starter project are here: https://github.com/solana-labs/wallet-adapter#build-from-source

I was not able to get the starter projects to work.

Since there is a parent directory to the src directory, nothing works if you build the project and then move the src to your other project. I don't want all the other stuff - literally just the front end app is what I want.

For example:

The instructions in https://github.com/solana-labs/wallet-adapter#build-from-source

Clone the project:

git clone https://github.com/solana-labs/wallet-adapter.git

Install dependencies:

cd wallet-adapter
yarn install

Build all packages:

yarn build

Run locally:

cd packages/starter/react-ui-starter
yarn start

Then I moved the react-ui-starter folder to my app, and it does not work, since it is dependent on the parent folder's node_modules

kevinforrestconnors commented 2 years ago

@Malai237 you don't need to downgrade the packages to get this to work with CRA 4. The react-ui-starter project works out of the box with the latest versions of the packages using CRA 4.

I can't even get this to work with CRA 4. Jest tests break:

    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){export * from './useWalletModal';
                                                                                             ^^^^^^

    SyntaxError: Unexpected token 'export'
kevinforrestconnors commented 2 years ago

So I have learned that the issue is specifically with WalletModalProvider breaking jest tests, even with CRA 4. I have decided to just not use it and make my own UI.

jordaaash commented 2 years ago

Then I moved the react-ui-starter folder to my app, and it does not work, since it is dependent on the parent folder's node_modules

This is because of Lerna + Yarn workspaces linking the dependencies locally.

If you move react-ui-starter out of the project directory, run yarn install from the react-ui-starter directory. This should install the dependencies locally in the node_modules of that directory instead of hoisting them.

jordaaash commented 2 years ago

Jest tests break

What tests are you running? The only Jest tests that ship with this project right now are in the React package. This shouldn't affect your starter project running, right?

jordaaash commented 2 years ago

As for the Jest tests that ship with the React package, they run and pass:

➜  react git:(master) yarn test
yarn run v1.22.17
$ jest
...
Test Suites: 2 passed, 2 total
Tests:       45 passed, 45 total
Snapshots:   0 total
Time:        6.412 s
Ran all test suites.
✨  Done in 7.91s.
jordaaash commented 2 years ago

If you move react-ui-starter out of the project directory, run yarn install from the react-ui-starter directory.

I just did this and it works.

jordaaash commented 2 years ago

Perhaps we should configure the starter projects to disable hoisting since they are supposed to run standalone. Comes at the expense of initial install time, but maybe worthwhile?

cc @cogoo

gpoole commented 2 years ago

Just to add my experience, the packages don't work with the latest Next.js version (12.0.7) in a project I created via CNA. Downgrading to Next.js 11 as used in the starter seems to have worked though. Next.js now uses SWC instead of webpack by default, so might this also be part of the problem? I'm not familiar with the details at all so please forgive me if this is a dumb question, but it seems like using mjs extensions for the esm exports has the broadest compatibility and resolves the issue, so is there a reason not to do that?

jordaaash commented 2 years ago

@gpoole https://github.com/solana-labs/wallet-adapter/issues/246 is probably related to the Next.js issue. I'm also not familiar with it and haven't tested with Next 12.

it seems like using mjs extensions for the esm exports has the broadest compatibility and resolves the issue, so is there a reason not to do that?

@cogoo you spent a good bit of time trying out different builds, wdyt?

cogoo commented 2 years ago

@gpoole #246 is probably related to the Next.js issue. I'm also not familiar with it and haven't tested with Next 12.

it seems like using mjs extensions for the esm exports has the broadest compatibility and resolves the issue, so is there a reason not to do that?

@cogoo you spent a good bit of time trying out different builds, wdyt?

It looks like most bundlers are now enforcing strict ESM imports which are where the errors occur. To my knowledge the .mjs extension wouldn't help since the ESM build is picked up correctly. The issue is that we are not using full file paths for imports.

i.e. import { someModule } from './module needs to be import { someModule } from './module.js

gpoole commented 2 years ago

I've mentioned this on #246 but I noticed in Next.js 12 just changing the extension to .mjs seemed to be enough. It looks like Next or maybe SWC is using esm loader which does say about its strict mode:

"strict" to treat only .mjs files as ESM.

So maybe that's it, or maybe I'm misreading. Looking around I noticed the Webpack docs say:

Imports in ESM are resolved more strictly. Relative requests must include a filename and file extension.

And the Node docs also say:

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.

I wasn't familiar with this change. Sorry for the diversion!

gpoole commented 2 years ago

I didn't originally understand your earlier comment about using ".js" for imports in ".ts" files @cogoo but now I realised that's the formal decision on how this is handled by TypeScript:

[code] will have to be rewritten to use the extension of the output of foo.ts - so bar.ts will instead have to import from ./foo.js.

I thought it was going to be fixed by a compiler option or something. Strange!

jordaaash commented 2 years ago

Unfortunately we tried changing the source code to do this, and it has a number of problems we don't have a good solution to yet. I think a better solution may be to find a way to output .mjs files in the ESM build, and also reference those extensions in the build, without modifying the TS source.

jordaaash commented 2 years ago

https://www.npmjs.com/package/typescript-esm / https://github.com/kristoferbaxter/typescript-esm looks promising

When you add the tsc-esm compiler following the conclusion of TypeScript's output, this compiler will remap all generated files to use '.mjs' extensions for locally resolved items and rename every output's extension to '.mjs'.

Sounds like what we want?

cogoo commented 2 years ago

https://www.npmjs.com/package/typescript-esm / https://github.com/kristoferbaxter/typescript-esm looks promising

When you add the tsc-esm compiler following the conclusion of TypeScript's output, this compiler will remap all generated files to use '.mjs' extensions for locally resolved items and rename every output's extension to '.mjs'.

Sounds like what we want?

This looks like it will do the job! I'll play around with this today

cogoo commented 2 years ago

https://www.npmjs.com/package/typescript-esm / https://github.com/kristoferbaxter/typescript-esm looks promising

When you add the tsc-esm compiler following the conclusion of TypeScript's output, this compiler will remap all generated files to use '.mjs' extensions for locally resolved items and rename every output's extension to '.mjs'.

Sounds like what we want?

Experimented with this. Unfortunately, it doesn't work out of the box. After installing the package, the lerna build script fails on the packages/starter/example repository;

lerna ERR! yarn run build exited 1 in '@solana/wallet-adapter-example'
lerna ERR! yarn run build stdout:
$ yarn clean && next build && next export
$ shx rm -rf .next out
info  - Using webpack 5. Reason: Enabled by default https://nextjs.org/docs/messages/webpack5
info  - Checking validity of types...
info  - Creating an optimized production build...
info  - Using external babel configuration from /Users/cogoo/LocalHost/GitHub/solana_projects/wallet-adapter/packages/starter/example/.babelrc.js
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! yarn run build stderr:
Failed to compile.

ModuleNotFoundError: Module not found: Error: Can't resolve '@solana/wallet-adapter-react-ui' in '/Users/cogoo/LocalHost/GitHub/solana_projects/wallet-adapter/packages/starter/example/pages'

> Build error occurred

I'll be more comfortable adding the extension manually (prebuild) as that's the accepted convention.

https://www.typescriptlang.org/docs/handbook/esm-node.html

CC: @gpoole

jordaaash commented 2 years ago

I've seen that build error before, I don't know that it's related to this or unresolvable.

For the reasons we discussed before, I'm not comfortable with adding the extension to the source. It increases maintenance burden and eslint doesn't work for it yet. I'm also not convinced yet that this is a reasonable convention to follow when I haven't seen it used a single time in the wild.

I'll pick this up and work on it today.

jordaaash commented 2 years ago

254 will fix this when merged.

jordaaash commented 2 years ago

Published:

These packages now have strict ESM exports (.mjs files). This fixes Next.js 12-based projects which use Webpack 5 with strict ESM imports.

It doesn't fix CRA 5-based projects because of a semi-related issue: Webpack 5 doesn't include polyfills for Node.js APIs by default, so it breaks with buffer and crypto imports.

Even though this issue was about strict ESM imports, I'll leave it open until CRA projects work.

jordaaash commented 2 years ago

To anyone looking for a very good workaround right now, don't use Create React App. Just use the nextjs-starter project, save yourself a huge hassle, and get even faster builds with SWC instead of Babel.

jordaaash commented 2 years ago

The react-ui-starter and material-ui-starter projects have been updated to use Parcel instead of CRA.

steveluscher commented 2 years ago

Can anyone having trouble with the Solana wallet adapter and Create React App 5 patch in #264 and let me know if that solves your build issues? If you find another missing module, let me know what it is (paste in the build failure message if possible).

cc/ @cogoo, @Eliascm17, @Malai237, @dan-garay, @kevinforrestconnors

steveluscher commented 2 years ago

It appears as though the CRA team's position is pretty clear: if we depend on a package and that package is designed to work in the browser, then they must produce a browser build that does not depend on Node APIs.

Read more about this here: https://github.com/facebook/create-react-app/issues/11756#issuecomment-996464456

In the meantime, some are suggesting to monkey patch your CRA5 Webpack config using react-app-rewired to add the Node polyfills back: https://github.com/facebook/create-react-app/issues/11756#issuecomment-1013246173. This is slightly less nuclear an option than ejecting from CRA or downgrading to CRA4.

jordaaash commented 2 years ago

Thanks for digging into this @steveluscher. I think react-app-rewired for now, or Craco once it supports CRA 5, is probably the way to go.

To anyone following this issue, my current recommendation is still just to use Parcel if you want something quick and lightweight (e.g. without server side rendering, which most dapps don't need), or Next.js if you want something heavier (SSR, API functions, etc.)

I realize that's not very satisfying, but unless you are already using some of the distinctive features of CRA like service workers, Jest, etc. you may not need it right away, and a simpler tool can help you build what you want.

jordaaash commented 2 years ago

Any volunteers want to try out @steveluscher's promising solution @ https://github.com/solana-labs/wallet-adapter/pull/264#issuecomment-1015981946

Would love to know if this works for more people!

AlexRubik commented 2 years ago

Any volunteers want to try out @steveluscher's promising solution @ #264 (comment)

Would love to know if this works for more people!

I'd be more than happy to test this, but I'm a bit ignorant to how. After I create-react-app, how do I install Steven's packages instead of "yarn add @solana/wallet-adapter-base ... etc." ?

jordaaash commented 2 years ago

@dewittalex thanks! @steveluscher probably has better advice than me, but I think if you build his branch from source and then locally reference the packages from your CRA app, it might work.

Maybe something like "@solana/wallet-adapter-react": "file:/../wallet-adapter/packages/core/react" for all the relevant packages you depend on?

And then you might want to use https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/ or something to make sure that the internal dependencies also resolve to those since the directory structure will be a bit weird.

jordaaash commented 2 years ago

Another option, actually -- stick your CRA package inside the starters directory of his branch after building it from source. Then you can run yarn from the base directory, and it should link your local dependencies without all that mess.

steveluscher commented 2 years ago

You can link local NPM packages and use them as though they're the published versions. See if you can get something like this working:

# Check out and build my PR
git clone https://github.com/steveluscher/wallet-adapter.git
cd wallet-adapter
git checkout polyfill-node-apis-for-webpack-5
yarn install && yarn build

# Link my PR's versions into your package manager
(cd packages/wallets/ledger && yarn link)
(cd packages/wallets/torus && yarn link)
(cd packages/wallets/wallets && yarn link)

Then go back to your project…

cd myproject
# Use the linked versions of these packages in your project
yarn link @solana/wallet-adapter-ledger
yarn link @solana/wallet-adapter-torus
yarn link @solana/wallet-adapter-wallets

And try to build.

You can unlink everything when you're done. Make sure to follow the instructions that appear.

yarn unlink @solana/wallet-adapter-ledger
yarn unlink @solana/wallet-adapter-torus
yarn unlink @solana/wallet-adapter-wallets
steveluscher commented 2 years ago

I just tried my own suggestion above and met with limited success. The Ledger wallet imported just fine, but the Torus wallet did not.

npx create-react-app my-app
cd my-app
yarn add @solana/wallet-adapter-base
yarn add @solana/wallet-adapter-react
yarn add @solana/wallet-adapter-wallets
yarn add @solana/web3.js
yarn link @solana/wallet-adapter-ledger
yarn link @solana/wallet-adapter-torus
yarn link @solana/wallet-adapter-wallets

This works fine:

import { WalletNotConnectedError } from "@solana/wallet-adapter-base";
import { useConnection, useWallet } from "@solana/wallet-adapter-react";
import LedgerWalletAdapter from "@solana/wallet-adapter-ledger";

But this results in an error finding stream:

import { WalletNotConnectedError } from "@solana/wallet-adapter-base";
import { useConnection, useWallet } from "@solana/wallet-adapter-react";
import TorusWalletAdapter from "@solana/wallet-adapter-torus";

I wonder if this is actually a problem with the PR, or if yarn link doesn't understand how to propagate that package.json dependency down to the main project. When I add it manually, everything works.

# Add to the `dependencies` section of the package.json in your Create React App app.
"stream": "npm:stream-browserify@^3.0.0",
steveluscher commented 2 years ago

…or if yarn link doesn't understand how to propagate that package.json dependency down to the main project.

I sort of suspect that's true: https://github.com/yarnpkg/yarn/issues/2914#issuecomment-358055934

Maybe we should #yolo merge this, publish it, and see?

jordaaash commented 2 years ago

Published:

jordaaash commented 2 years ago

(And if this doesn't fix all the CRA issues, at least we're a bit closer)

jordaaash commented 2 years ago

Reopened -- #264 has been reverted by #275. It looks like the polyfills may be breaking the Next and Parcel starters.