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

0.6.0 broke Vite dev mode #82

Closed
gunters63 opened this issue Jun 19, 2023 · 26 comments · Fixed by #93
Closed

0.6.0 broke Vite dev mode #82

gunters63 opened this issue Jun 19, 2023 · 26 comments · Fixed by #93

Comments

@gunters63
Copy link

If i update jotai-devtools to 0.6.0 my UI breaks with the following message in the console:

image

Chalk is an npm package which works only in a Node environment, hence process is not defined.

If I look at the stacktrace, the error comes from this origin:

image

Time travel is a new feature in devtools 0.6 if I understand correctly.

Could it be that some of the imported npm packages there use chalk internally? Maybe @mantine/core?

@gunters63
Copy link
Author

The import of chalk seems to come from jsondiffpatch:

image

I had to disable javascript source maps to really see this

@gunters63
Copy link
Author

The problematic import seems to be here:

image

jsondiffpatch has a static import of chalk which causes the crash

@gunters63
Copy link
Author

Maybe polyfilling the process node module can help, I will try.

Not sure if other bundlers will have similar problems.

@gunters63
Copy link
Author

vite-plugin-node-polyfills helped

@aroman
Copy link

aroman commented Jun 20, 2023

also running into this issue.

by sheer coincidence, last week we needed to build json diff/patching into our project. we ran into issues with jsondiffpatch for exactly this reason, and instead moved to fast-json-patch, which is a much cleaner and more modern API. it's typescript-native. might be worth considering!

@arjunvegda
Copy link
Member

Thanks for reaching out and for the investigation! It's super helpful

I'm currently away from keyboard for a week, I'll look into this in detail sometime next week.

Meanwhile, is this happening only with Vite? Or are we seeing issues with Rollup and other bundlers too?

@arjunvegda
Copy link
Member

This issue is logged on their repo too. There are a few workarounds you may look into btw.

for exactly this reason, and instead moved to fast-json-patch, which is a much cleaner and more modern API.

Thanks for the recommendation. fast-json-patch looks good, I like that it follows the RFC 6902 standard. Would you be open to creating a PR?

@aroman
Copy link

aroman commented Jun 27, 2023

i did try to give it a shot, but for some reason when I use the local version of jotai-devtools, it's missing all the atoms...

any tips on getting jotai-devtools to work locally?

what I did was:

pnpm install
pnpm compile

and then I referenced it in my project's package.json like:

"jotai-devtools": "../jotai-devtools"

I should note that version 0.5.3 from npm works fine, and even version 0.6.0 works fine via npm, using the polyfill plugin that gunters63 mentioned.

CleanShot 2023-06-27 at 02 05 30@2x

@arjunvegda
Copy link
Member

Ah sorry, for the lack of a contribution guide 😅. We use storybook to develop this extension.

Here are steps to get you up and running locally

  • pnpm install
  • pnpm storybook
  • Use DevTools.stories.tsx as a playground or Playground.stories.tsx to test your changes

aroman added a commit to magic-circle-studio/jotai-devtools that referenced this issue Jun 28, 2023
aroman added a commit to magic-circle-studio/jotai-devtools that referenced this issue Jun 28, 2023
@aroman
Copy link

aroman commented Jun 28, 2023

so, i did a bit more digging here. tl;dr i don't think we should use fast-json-patch, and instead should just ship a trivial patch to jsondiffpatch's chalk dependency to get it to play nice with vite (i.e. not reference process).

I've created a PR #85 which implements this patch using pnpm.

I can confirm that my patch at least prevents jotai-devtools from from totally breaking vite, but I can't actually test the functionality because I still get that issue where all the atoms are missing when I install via npm link jotai-devtools. I can confirm that it does work locally via pnpm storybook, and it passes all the tests.

More context:
RFC 6902 does not include the previous values in the delta operations it generates. this doesn't matter for what I need it for in my project, but now that i've dug into how JSON diffing is being used in this project, it's basically the whole point, making the package of little value (unless you want to forego the entire "diff" view feature, which I don't think you do).

Besides that, looking at the code, it's very tightly coupled to the diff format produced by jsondiffpatch. there's a lot of elaborate string and object manipulation code, all with any types (since jsondiffpatch doesn't include types)... so even if we found an alternative json diff library, unless it happens to output in the same non-standard format as jsondiffpatch... it would be a big undertaking to rework everything to use the new format.

Given that... I think the best option is simply using jsondiffpatch and patching it to avoid the process call. Turns out that's a trivial one-line patch, as someone pointed out here.

@arjunvegda
Copy link
Member

Thanks for your investigation and PR!

I'm not very confident in shipping a patch via jotai-devtools 😅 given that our users have multiple workarounds (define process.platform on their end, patch jsondiffpatch on userland, etc.). Besides, based on your patch I wonder if it would work for non-pnpm package managers 🤔

Perhaps, you may be interested in creating a PR in the jsondiffpatch repo for this issue 😄

I can't actually test the functionality because I still get that issue where all the atoms are missing when I install via npm link jotai-devtools.

We have a canary deployment setup for PRs, here are the instructions on how to install the canary version generated using your PR.

@aroman
Copy link

aroman commented Jun 29, 2023

agreed with you! the pnpm patching system was new to me too — i tried the canary package and it fails with the same chalk/process issue, so looks like the pnpm patch isn't coming through.

I do feel that this is something that should be fixed in jotai-devtools, or at the very least documented, since ultimately jotai-devtools is bundling a dependency with a show-stopping bug in it for vite users.

i agree that the ideal outcome is jsondiffpatch fixes this, but it seems that package has been abandoned since 2021, so I think a PR will fall on deaf ears :(

@arjunvegda
Copy link
Member

Yes, ideally it'd be nice to address it in jotai-devtools. Perhaps we could use jsondiffpatch-rc until a long-term fix is identified. 🤔 I'll try to brainstorm other ideas.

but it seems that package has been abandoned since 2021

The author of the jsondiffpatch library is open to having a volunteer. Perhaps there is a beam of hope 😅 .

Chalk dropped the process check in chalk@>=3.0.0, so maybe the PR would be slightly less complex. WDYT?

@arjunvegda
Copy link
Member

Perhaps we could use benjamine/jsondiffpatch#315 (comment) until a long-term fix is identified. 🤔 I'll try to brainstorm other ideas.

I tried this! Unfortunately, the build fails due to some import error.


I created a PR that should fix this on the jsondiffpatch repo 🤞

@maylortaylor
Copy link

Writing here to say that I also am experiencing this issue. I really hope that PR for jsondiffpatch goes through and is a solution to this. Would really love to use the DevTools soon!

My project's setup is

  • react 18.2.0
  • jotai 2.2.2
  • jotai-devtools 0.6.0
  • vite 4.3.9

@arjunvegda
Copy link
Member

Hey @maylortaylor, just to clarify further, this issue does not completely block you from using DevTools. DevTools does not rely on the code that's causing this error 😁

Here is a workaround you may use in your project. Define process.platform in your vite.config.ts file until the PR with the fix is merged.

export default defineConfig({
  // ...
  define: {
    'process.platform': '' // you may set this to anything
  }
})

@maylortaylor
Copy link

Hey @maylortaylor, just to clarify further, this issue does not completely block you from using DevTools. DevTools does not rely on the code that's causing this error 😁

Here is a workaround you may use in your project. Define process.platform in your vite.config.ts file until the PR with the fix is merged.

export default defineConfig({
  // ...
  define: {
    'process.platform': '' // you may set this to anything
  }
})

Thanks so much @arjunvegda! I set 'process.platform': null and it started working just fine.
One question though, is there a way move the Jotai Devtools icon or is there a plan to make a chrome extension in the future? The icon is covering some of my UI that I need.

I would also suggest adding a new prop on the Devtools that would set it's placement (top-right, bottom-left, etc)

@arjunvegda
Copy link
Member

Great suggestions! @maylortaylor. Let's continue this discussion in #86

@jokereven
Copy link

.

I choose to use version 0.5.3

@samchouse
Copy link

samchouse commented Aug 16, 2023

Hey @maylortaylor, just to clarify further, this issue does not completely block you from using DevTools. DevTools does not rely on the code that's causing this error 😁

Here is a workaround you may use in your project. Define process.platform in your vite.config.ts file until the PR with the fix is merged.

export default defineConfig({
  // ...
  define: {
    'process.platform': '' // you may set this to anything
  }
})

Just an fyi, this doesn't work with Vitest because then Vitest can't set process.env while testing. On top of that vite-plugin-node-polyfills causes a lot of terminal output while transforming. So the only way is patching for me atleast.

Edit: There's actually an even better way to do this, which is overriding packages. Add the following in your package.json. I had to do a clean install after for it to work.
NPM:

  "overrides": {
    "jotai-devtools": {
      "chalk": "^4.1.2"
    }
  }

Yarn:

  "resolutions": {
    "jotai-devtools/**/chalk": "^4.1.2"
  }

PNPM:

  "pnpm": {
    "overrides": {
      "jotai-devtools>chalk": "^4.1.2"
    }
  }

Note: I've only tested with NPM.

@arjunvegda
Copy link
Member

@Xenfo Thanks for sharing your solution! 🙏

Out of curiosity, why do we need Jotai DevTools when running tests? 🤔

@samchouse
Copy link

samchouse commented Aug 17, 2023

We don't but the define block in the Vite config overrides process.platform for all environments, meaning it'll override it when testing with Vitest which causes it to crash and fail.

@arjunvegda
Copy link
Member

arjunvegda commented Aug 17, 2023

Wouldn't it be safer to wrap <DevTools/> with a process.env.NODE_ENV === 'development' check then? 🤔

@aroman
Copy link

aroman commented Aug 17, 2023

Edit: There's actually an even better way to do this, which is overriding packages. Add the following in your package.json. I had to do a clean install after for it to work.

this worked a charm! for me, using pnpm, what I had to do was:

"pnpm": {
  "overrides": {
    "jsondiffpatch>chalk": "^4.1.2"
  }
}

@samchouse
Copy link

samchouse commented Aug 17, 2023

Wouldn't it be safer to wrap <DevTools/> with a process.env.NODE_ENV === 'development' check then? 🤔

Jotai Devtools isn't part of any test code but the define block applies to all environments (including Vitest).

export default defineConfig({
  // ...
  define: {
    'process.platform': '' // you may set this to anything
  }
})

This define block will make process.platform readonly (probably by setting it to a const) and Vitest needs to be able to set process.platform to run. So it's not actually an issue with the library, just the workaround.

@arjunvegda
Copy link
Member

Fix released in v0.6.2 🚀

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 a pull request may close this issue.

6 participants