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

Diff not parsing for added file #31

Open
kentcdodds opened this issue Sep 17, 2024 · 5 comments · Fixed by #32
Open

Diff not parsing for added file #31

kentcdodds opened this issue Sep 17, 2024 · 5 comments · Fixed by #32

Comments

@kentcdodds
Copy link

Here's a test case:

import parseGitDiff from 'parse-git-diff'

const diffOutput = `diff --git a/var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/04.01.solution/7h2jowvfi2q/index.test.tsx b/var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/04.01.solution/7h2jowvfi2q/index.test.tsx
new file mode 100644
index 0000000..e69de29
diff --git a/var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/04.01.problem/7h2jowvfi2q/index.tsx b/var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/04.01.solution/7h2jowvfi2q/index.tsx
index 9913856..4d68325 100644
--- a/var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/04.01.problem/7h2jowvfi2q/index.tsx
+++ b/var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/04.01.solution/7h2jowvfi2q/index.tsx
@@ -1,4 +1,4 @@
-import { useCallback, useEffect, useState } from 'react'
+import { createContext, useEffect, useState, use, useCallback } from 'react'
 import * as ReactDOM from 'react-dom/client'
 import {
        type BlogPost,
@@ -7,15 +7,16 @@ import {
 } from '#shared/blog-posts'
 import { setGlobalSearchParams } from '#shared/utils'
 
-// 🦺 create a SearchParamsTuple type here that's a readonly array of two elements:
-// - the first element is a URLSearchParams instance
-// - the second element is typeof setGlobalSearchParams
-// 🐨 create a SearchParamsContext that is of this type
-// 💰 let's start with this as the default value (we'll improve it next):
-// [new URLSearchParams(window.location.search), setGlobalSearchParams]
+type SearchParamsTuple = readonly [
+       URLSearchParams,
+       typeof setGlobalSearchParams,
+]
+const SearchParamsContext = createContext<SearchParamsTuple>([
+       new URLSearchParams(window.location.search),
+       setGlobalSearchParams,
+])
 
-// 🐨 change this to SearchParamsProvider and accept children
-function useSearchParams() {
+function SearchParamsProvider({ children }: { children: React.ReactNode }) {
        const [searchParams, setSearchParamsState] = useState(
                () => new URLSearchParams(window.location.search),
        )
@@ -46,23 +47,29 @@ function useSearchParams() {
                [],
        )
 
-       // 🐨 instead of returning this, render the SearchParamsContext and
-       // provide this tuple as the value
-       // 💰 make sure to render the children as well!
-       return [searchParams, setSearchParams] as const
+       const searchParamsTuple = [searchParams, setSearchParams] as const
+
+       return (
+               <SearchParamsContext value={searchParamsTuple}>
+                       {children}
+               </SearchParamsContext>
+       )
 }
 
-// 🐨 create a useSearchParams hook here that returns use(SearchParamsContext)
+function useSearchParams() {
+       return use(SearchParamsContext)
+}
 
 const getQueryParam = (params: URLSearchParams) => params.get('query') ?? ''
 
 function App() {
        return (
-               // 🐨 wrap this in the SearchParamsProvider
-               <div className="app">
-                       <Form />
-                       <MatchingPosts />
-               </div>
+               <SearchParamsProvider>
+                       <div className="app">
+                               <Form />
+                               <MatchingPosts />
+                       </div>
+               </SearchParamsProvider>
        )
 }
 `

const parsed = parseGitDiff(diffOutput)
console.log(parsed)

// outputs: { type: 'GitDiff', files: [] }

I'm not certain why the index.test.tsx file is causing an issue here, but if I remove that from the diff the rest parses just fine.

@yeonjuan
Copy link
Owner

@kentcdodds Thank you for your report. I have confirmed the reproduction of the bug.

@yeonjuan
Copy link
Owner

Hi, @kentcdodds Could you try v0.0.16?

@kentcdodds
Copy link
Author

It's better! Now the diff is showing files, however it's saying that a file which was modified was actually added. Here's my diff example:

diff --git var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/09.03.solution/dn2ncwjsbmo/index.test.ts var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/09.03.solution/dn2ncwjsbmo/index.test.ts
new file mode 100644
index 0000000..e69de29
diff --git var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/playground/dn2ncwjsbmo/index.tsx var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/09.03.solution/dn2ncwjsbmo/index.tsx
index 4d68325..fd576f7 100644
--- var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/playground/dn2ncwjsbmo/index.tsx
+++ var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/09.03.solution/dn2ncwjsbmo/index.tsx
@@ -1,190 +1,54 @@
-import { createContext, useEffect, useState, use, useCallback } from 'react'
+import { Suspense, useSyncExternalStore } from 'react'
 import * as ReactDOM from 'react-dom/client'
-import {
-       type BlogPost,
-       generateGradient,
-       getMatchingPosts,
-} from '#shared/blog-posts'
-import { setGlobalSearchParams } from '#shared/utils'
 
-type SearchParamsTuple = readonly [
-       URLSearchParams,
-       typeof setGlobalSearchParams,
-]
-const SearchParamsContext = createContext<SearchParamsTuple>([
-       new URLSearchParams(window.location.search),
-       setGlobalSearchParams,
-])
-
-function SearchParamsProvider({ children }: { children: React.ReactNode }) {
-       const [searchParams, setSearchParamsState] = useState(
-               () => new URLSearchParams(window.location.search),
-       )
+export function makeMediaQueryStore(mediaQuery: string) {
+       function getSnapshot() {
+               return window.matchMedia(mediaQuery).matches
+       }
 
-       useEffect(() => {
-               function updateSearchParams() {
-                       setSearchParamsState((prevParams) => {
-                               const newParams = new URLSearchParams(window.location.search)
-                               return prevParams.toString() === newParams.toString()
-                                       ? prevParams
-                                       : newParams
-                       })
+       function subscribe(callback: () => void) {
+               const mediaQueryList = window.matchMedia(mediaQuery)
+               mediaQueryList.addEventListener('change', callback)
+               return () => {
+                       mediaQueryList.removeEventListener('change', callback)
                }
-               window.addEventListener('popstate', updateSearchParams)
-               return () => window.removeEventListener('popstate', updateSearchParams)
-       }, [])
-
-       const setSearchParams = useCallback(
-               (...args: Parameters<typeof setGlobalSearchParams>) => {
-                       const searchParams = setGlobalSearchParams(...args)
-                       setSearchParamsState((prevParams) => {
-                               return prevParams.toString() === searchParams.toString()
-                                       ? prevParams
-                                       : searchParams
-                       })
-                       return searchParams
-               },
-               [],
-       )
-
-       const searchParamsTuple = [searchParams, setSearchParams] as const
-
-       return (
-               <SearchParamsContext value={searchParamsTuple}>
-                       {children}
-               </SearchParamsContext>
-       )
-}
-
-function useSearchParams() {
-       return use(SearchParamsContext)
-}
-
-const getQueryParam = (params: URLSearchParams) => params.get('query') ?? ''
-
-function App() {
-       return (
-               <SearchParamsProvider>
-                       <div className="app">
-                               <Form />
-                               <MatchingPosts />
-                       </div>
-               </SearchParamsProvider>
-       )
-}
-
-function Form() {
-       const [searchParams, setSearchParams] = useSearchParams()
-       const query = getQueryParam(searchParams)
-
-       const words = query.split(' ').map((w) => w.trim())
-
-       const dogChecked = words.includes('dog')
-       const catChecked = words.includes('cat')
-       const caterpillarChecked = words.includes('caterpillar')
-
-       function handleCheck(tag: string, checked: boolean) {
-               const newWords = checked ? [...words, tag] : words.filter((w) => w !== tag)
-               setSearchParams(
-                       { query: newWords.filter(Boolean).join(' ').trim() },
-                       { replace: true },
-               )
        }
 
-       return (
-               <form onSubmit={(e) => e.preventDefault()}>
-                       <div>
-                               <label htmlFor="searchInput">Search:</label>
-                               <input
-                                       id="searchInput"
-                                       name="query"
-                                       type="search"
-                                       value={query}
-                                       onChange={(e) =>
-                                               setSearchParams({ query: e.currentTarget.value }, { replace: true })
-                                       }
-                               />
-                       </div>
-                       <div>
-                               <label>
-                                       <input
-                                               type="checkbox"
-                                               checked={dogChecked}
-                                               onChange={(e) => handleCheck('dog', e.currentTarget.checked)}
-                                       />{' '}
-                                       🐶 dog
-                               </label>
-                               <label>
-                                       <input
-                                               type="checkbox"
-                                               checked={catChecked}
-                                               onChange={(e) => handleCheck('cat', e.currentTarget.checked)}
-                                       />{' '}
-                                       🐱 cat
-                               </label>
-                               <label>
-                                       <input
-                                               type="checkbox"
-                                               checked={caterpillarChecked}
-                                               onChange={(e) =>
-                                                       handleCheck('caterpillar', e.currentTarget.checked)
-                                               }
-                                       />{' '}
-                                       🐛 caterpillar
-                               </label>
-                       </div>
-               </form>
-       )
+       return function useMediaQuery() {
+               return useSyncExternalStore(subscribe, getSnapshot)
+       }
 }
 
-function MatchingPosts() {
-       const [searchParams] = useSearchParams()
-       const query = getQueryParam(searchParams)
-       const matchingPosts = getMatchingPosts(query)
+const useNarrowMediaQuery = makeMediaQueryStore('(max-width: 600px)')
 
-       return (
-               <ul className="post-list">
-                       {matchingPosts.map((post) => (
-                               <Card key={post.id} post={post} />
-                       ))}
-               </ul>
-       )
+function NarrowScreenNotifier() {
+       const isNarrow = useNarrowMediaQuery()
+       return isNarrow ? 'You are on a narrow screen' : 'You are on a wide screen'
 }
 
-function Card({ post }: { post: BlogPost }) {
-       const [isFavorited, setIsFavorited] = useState(false)
+function App() {
        return (
-               <li>
-                       {isFavorited ? (
-                               <button
-                                       aria-label="Remove favorite"
-                                       onClick={() => setIsFavorited(false)}
-                               >
-                                       ❤️
-                               </button>
-                       ) : (
-                               <button aria-label="Add favorite" onClick={() => setIsFavorited(true)}>
-                                       🤍
-                               </button>
-                       )}
-                       <div
-                               className="post-image"
-                               style={{ background: generateGradient(post.id) }}
-                       />
-                       <a
-                               href={post.id}
-                               onClick={(event) => {
-                                       event.preventDefault()
-                                       alert(`Great! Let's go to ${post.id}!`)
-                               }}
-                       >
-                               <h2>{post.title}</h2>
-                               <p>{post.description}</p>
-                       </a>
-               </li>
+               <div>
+                       <div>This is your narrow screen state:</div>
+                       <Suspense fallback="">
+                               <NarrowScreenNotifier />
+                       </Suspense>
+               </div>
        )
 }
 
 const rootEl = document.createElement('div')
 document.body.append(rootEl)
-ReactDOM.createRoot(rootEl).render(<App />)
+// 🦉 here's how we pretend we're server-rendering
+rootEl.innerHTML = (await import('react-dom/server')).renderToString(<App />)
+
+// 🦉 here's how we simulate a delay in hydrating with client-side js
+await new Promise((resolve) => setTimeout(resolve, 1000))
+
+ReactDOM.hydrateRoot(rootEl, <App />, {
+       onRecoverableError(error) {
+               if (String(error).includes('Missing getServerSnapshot')) return
+
+               console.error(error)
+       },
+})

The result is:

{
  type: 'GitDiff',
  files: [
    {
      type: 'DeletedFile',
      chunks: [Array],
      path: 'var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/playground/dn2ncwjsbmo/index.css'
    },
    {
      type: 'AddedFile',
      chunks: [Array],
      path: 'var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/09.03.solution/dn2ncwjsbmo/index.tsx'
    }
  ]
}

If I remove the empty file, then the diff is:

diff --git var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/playground/is245u03h1s/index.tsx var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/09.03.solution/is245u03h1s/index.tsx
index 4d68325..fd576f7 100644
--- var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/playground/is245u03h1s/index.tsx
+++ var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/09.03.solution/is245u03h1s/index.tsx
@@ -1,190 +1,54 @@
-import { createContext, useEffect, useState, use, useCallback } from 'react'
+import { Suspense, useSyncExternalStore } from 'react'
 import * as ReactDOM from 'react-dom/client'
-import {
-       type BlogPost,
-       generateGradient,
-       getMatchingPosts,
-} from '#shared/blog-posts'
-import { setGlobalSearchParams } from '#shared/utils'
 
-type SearchParamsTuple = readonly [
-       URLSearchParams,
-       typeof setGlobalSearchParams,
-]
-const SearchParamsContext = createContext<SearchParamsTuple>([
-       new URLSearchParams(window.location.search),
-       setGlobalSearchParams,
-])
-
-function SearchParamsProvider({ children }: { children: React.ReactNode }) {
-       const [searchParams, setSearchParamsState] = useState(
-               () => new URLSearchParams(window.location.search),
-       )
+export function makeMediaQueryStore(mediaQuery: string) {
+       function getSnapshot() {
+               return window.matchMedia(mediaQuery).matches
+       }
 
-       useEffect(() => {
-               function updateSearchParams() {
-                       setSearchParamsState((prevParams) => {
-                               const newParams = new URLSearchParams(window.location.search)
-                               return prevParams.toString() === newParams.toString()
-                                       ? prevParams
-                                       : newParams
-                       })
+       function subscribe(callback: () => void) {
+               const mediaQueryList = window.matchMedia(mediaQuery)
+               mediaQueryList.addEventListener('change', callback)
+               return () => {
+                       mediaQueryList.removeEventListener('change', callback)
                }
-               window.addEventListener('popstate', updateSearchParams)
-               return () => window.removeEventListener('popstate', updateSearchParams)
-       }, [])
-
-       const setSearchParams = useCallback(
-               (...args: Parameters<typeof setGlobalSearchParams>) => {
-                       const searchParams = setGlobalSearchParams(...args)
-                       setSearchParamsState((prevParams) => {
-                               return prevParams.toString() === searchParams.toString()
-                                       ? prevParams
-                                       : searchParams
-                       })
-                       return searchParams
-               },
-               [],
-       )
-
-       const searchParamsTuple = [searchParams, setSearchParams] as const
-
-       return (
-               <SearchParamsContext value={searchParamsTuple}>
-                       {children}
-               </SearchParamsContext>
-       )
-}
-
-function useSearchParams() {
-       return use(SearchParamsContext)
-}
-
-const getQueryParam = (params: URLSearchParams) => params.get('query') ?? ''
-
-function App() {
-       return (
-               <SearchParamsProvider>
-                       <div className="app">
-                               <Form />
-                               <MatchingPosts />
-                       </div>
-               </SearchParamsProvider>
-       )
-}
-
-function Form() {
-       const [searchParams, setSearchParams] = useSearchParams()
-       const query = getQueryParam(searchParams)
-
-       const words = query.split(' ').map((w) => w.trim())
-
-       const dogChecked = words.includes('dog')
-       const catChecked = words.includes('cat')
-       const caterpillarChecked = words.includes('caterpillar')
-
-       function handleCheck(tag: string, checked: boolean) {
-               const newWords = checked ? [...words, tag] : words.filter((w) => w !== tag)
-               setSearchParams(
-                       { query: newWords.filter(Boolean).join(' ').trim() },
-                       { replace: true },
-               )
        }
 
-       return (
-               <form onSubmit={(e) => e.preventDefault()}>
-                       <div>
-                               <label htmlFor="searchInput">Search:</label>
-                               <input
-                                       id="searchInput"
-                                       name="query"
-                                       type="search"
-                                       value={query}
-                                       onChange={(e) =>
-                                               setSearchParams({ query: e.currentTarget.value }, { replace: true })
-                                       }
-                               />
-                       </div>
-                       <div>
-                               <label>
-                                       <input
-                                               type="checkbox"
-                                               checked={dogChecked}
-                                               onChange={(e) => handleCheck('dog', e.currentTarget.checked)}
-                                       />{' '}
-                                       🐶 dog
-                               </label>
-                               <label>
-                                       <input
-                                               type="checkbox"
-                                               checked={catChecked}
-                                               onChange={(e) => handleCheck('cat', e.currentTarget.checked)}
-                                       />{' '}
-                                       🐱 cat
-                               </label>
-                               <label>
-                                       <input
-                                               type="checkbox"
-                                               checked={caterpillarChecked}
-                                               onChange={(e) =>
-                                                       handleCheck('caterpillar', e.currentTarget.checked)
-                                               }
-                                       />{' '}
-                                       🐛 caterpillar
-                               </label>
-                       </div>
-               </form>
-       )
+       return function useMediaQuery() {
+               return useSyncExternalStore(subscribe, getSnapshot)
+       }
 }
 
-function MatchingPosts() {
-       const [searchParams] = useSearchParams()
-       const query = getQueryParam(searchParams)
-       const matchingPosts = getMatchingPosts(query)
+const useNarrowMediaQuery = makeMediaQueryStore('(max-width: 600px)')
 
-       return (
-               <ul className="post-list">
-                       {matchingPosts.map((post) => (
-                               <Card key={post.id} post={post} />
-                       ))}
-               </ul>
-       )
+function NarrowScreenNotifier() {
+       const isNarrow = useNarrowMediaQuery()
+       return isNarrow ? 'You are on a narrow screen' : 'You are on a wide screen'
 }
 
-function Card({ post }: { post: BlogPost }) {
-       const [isFavorited, setIsFavorited] = useState(false)
+function App() {
        return (
-               <li>
-                       {isFavorited ? (
-                               <button
-                                       aria-label="Remove favorite"
-                                       onClick={() => setIsFavorited(false)}
-                               >
-                                       ❤️
-                               </button>
-                       ) : (
-                               <button aria-label="Add favorite" onClick={() => setIsFavorited(true)}>
-                                       🤍
-                               </button>
-                       )}
-                       <div
-                               className="post-image"
-                               style={{ background: generateGradient(post.id) }}
-                       />
-                       <a
-                               href={post.id}
-                               onClick={(event) => {
-                                       event.preventDefault()
-                                       alert(`Great! Let's go to ${post.id}!`)
-                               }}
-                       >
-                               <h2>{post.title}</h2>
-                               <p>{post.description}</p>
-                       </a>
-               </li>
+               <div>
+                       <div>This is your narrow screen state:</div>
+                       <Suspense fallback="">
+                               <NarrowScreenNotifier />
+                       </Suspense>
+               </div>
        )
 }
 
 const rootEl = document.createElement('div')
 document.body.append(rootEl)
-ReactDOM.createRoot(rootEl).render(<App />)
+// 🦉 here's how we pretend we're server-rendering
+rootEl.innerHTML = (await import('react-dom/server')).renderToString(<App />)
+
+// 🦉 here's how we simulate a delay in hydrating with client-side js
+await new Promise((resolve) => setTimeout(resolve, 1000))
+
+ReactDOM.hydrateRoot(rootEl, <App />, {
+       onRecoverableError(error) {
+               if (String(error).includes('Missing getServerSnapshot')) return
+
+               console.error(error)
+       },
+})

And the parsed result is as expected:

{
  type: 'GitDiff',
  files: [
    {
      type: 'DeletedFile',
      chunks: [Array],
      path: 'var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/playground/is245u03h1s/index.css'
    },
    {
      type: 'ChangedFile',
      chunks: [Array],
      path: 'var/folders/kt/zd3bfncd0c3gjx25hbcq483c0000gn/T/epicshop/diff/advanced-react-apis/09.03.solution/is245u03h1s/index.tsx'
    }
  ]
}

@yeonjuan yeonjuan reopened this Sep 19, 2024
@christianalfoni
Copy link

Hi there :)

Thanks a ton for building this library! 🤗

I experience a similar issue, though it is quite simple:

  • If you add an empty file there will not be any parsed files
  • If you add a file with a change it will parse the file as an added file with chunks

Let me know if you want some help with the library, happy to create a PR, but want to ask what the intention is with the parseChangeMarkers, as this returns null for added files with no changes.

@yeonjuan
Copy link
Owner

Hi @christianalfoni
Thank you. Sorry for the late confirmation, let's see if we can fix this. PR is welcomed.

want to ask what the intention is with the parseChangeMarkers, as this returns null for added files with no changes.
The parseChangeMarkers is for parsing the part below, could you please share the git diff of the bug case?

--- a/rename.js
+++ b/rename.js

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.

3 participants