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

fix: yarn notifier #149

Merged
merged 3 commits into from
Dec 18, 2021
Merged

fix: yarn notifier #149

merged 3 commits into from
Dec 18, 2021

Conversation

johnrees
Copy link
Contributor

@johnrees johnrees commented Dec 17, 2021

Problem

yarn notifier was unable to run for me locally.

issue 1 - tsconfig baseUrl/paths

I was getting an error that it cannot find module stores/useWalletStore

Screenshot 2021-12-17 at 7 19 03 PM

I fixed this by adding `tsconfig-paths` into the tsconfig
 "ts-node": { 
+   "require": ["tsconfig-paths/register"], 
    "compilerOptions": { 
      "module": "commonjs" 
    } 
 } 

I also removed tsconfig.commonjs.json and merged it into the ts-node field in tsconfig.json

issue 2 - unexpected token '<'

After fixing the paths I got new errors because there is JSX in the one of the imports used by the notifier

Screenshot 2021-12-17 at 7 20 26 PM

I added react-jsx so that the JSX would be compiled rather than preserved. I didn't love this solution because I don't think there should be JSX loaded in a headless script that's called from node.

"compilerOptions": {
+  "jsx": "react-jsx",
  "module": "commonjs"
}

issue 3 - @solana/wallet-adapter-phantom is not cjs

The next error was because @solana/wallet-adapter-phantom is only exported as esmodules

Screenshot 2021-12-17 at 7 22 23 PM

I went down various rabbitholes trying to fix this (type: module in package.json, babel presets, swc, esbuild)

Solution 🎉

By far the simplest approach was to move ConnectionContext and getConnectionContext into their own file, this way the notifier script doesn't import any JSX or the esm @solana/wallet-adapter-phantom code.

⚠️ I don't know if utils/connection.ts is the best place for this file, or if it would be better to leave it in the store file and move the JSX and wallet-adapter code out of the store file instead?

But this works for now and fixes the immediate problem.

Finally

I also upgraded all eslint* dependencies because I got a warning from yarn test-all but I can revert that commit if it's problematic.

@vercel
Copy link

vercel bot commented Dec 17, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/solana-labs/governance-ui/7Jt7NPr9CnanVQMmfDdUEWZaY1LZ
✅ Preview: https://governance-ui-git-jr-fix-notifier-solana-labs.vercel.app

package.json Show resolved Hide resolved
@SebastianBor
Copy link
Collaborator

⚠️ I don't know if utils/connection.ts is the best place for this file, or if it would be better to leave it in the store file and move the JSX and wallet-adapter code out of the store file instead?

It's better here, I think store is inherently related to UI and connection is more basic concept
Just let's unify our utility functions under tools and deprecate utils

Thank you for all that cleanup work, the project needs it !!!

@SebastianBor SebastianBor merged commit c03f8e3 into main Dec 18, 2021
@johnrees johnrees deleted the jr/fix-notifier branch December 18, 2021 10:46
johnrees added a commit to johnrees/governance-ui that referenced this pull request Dec 18, 2021
was able to be simplified after solana-labs#149 was merged
johnrees added a commit to johnrees/governance-ui that referenced this pull request Dec 19, 2021
was able to be simplified after solana-labs#149 was merged
johnrees added a commit to johnrees/governance-ui that referenced this pull request Dec 19, 2021
was able to be simplified after solana-labs#149 was merged
johnrees added a commit to johnrees/governance-ui that referenced this pull request Dec 19, 2021
was able to be simplified after solana-labs#149 was merged
johnrees added a commit to johnrees/governance-ui that referenced this pull request Dec 20, 2021
was able to be simplified after solana-labs#149 was merged
johnrees added a commit to johnrees/governance-ui that referenced this pull request Dec 22, 2021
was able to be simplified after solana-labs#149 was merged
SebastianBor pushed a commit that referenced this pull request Dec 23, 2021
* remove unused testUtils

* update jest, rename .babelrc to babel.config.js

renaming babel config is required for jest to be able to transform es6 libs (like solana/wallet-adapter-phantom) https://stackoverflow.com/a/54656593

* test: update jest to handle tsconfig baseUrl/paths settings

* test: set jest testEnvironment to jsdom (rather than node)

* test: let `babel-jest` handle js(x) as well as ts(x) files

* test: add simple test to verify test settings

* test: simplify jest/babel setup

was able to be simplified after #149 was merged

* test: faster tests with swc/jest vs babel-jest

* test: use `next/jest`

- remove manual file mocks as they're handled by next/jest
- create test/setup.js file
- remove target config https://nextjs.org/docs/messages/deprecated-target-config

* test: test homepage redirects
johnrees added a commit to johnrees/governance-ui that referenced this pull request Dec 23, 2021
was able to be simplified after solana-labs#149 was merged
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

Successfully merging this pull request may close these issues.

None yet

2 participants