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

bug: name collision in isCellEditable breaks edit feature #3356

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

PenguinOfWar
Copy link
Contributor

@PenguinOfWar PenguinOfWar commented Oct 5, 2023

Issue

Under certain conditions when bundled, two identically named functions (isCellEditable) can clash.

How to replicate

  1. Import the library into next@13.5.4 or later
  2. Configure an editable cell per the common example
  3. npm run dev shows expected functionality of cell becoming editable after double click or carriage return
  4. npm run build && npm run start shows an unexpected outcome, cell edit becomes unreachable in production

Going through the compiled code with some strategic logging shows that the production build version of the code prefers the utility isCellEditable rather than the function inside DataGrid. As they accept two different arguments sets, this breaks the edit feature.

Fix

Renaming the utility to isCellEditableUtil resolves this issue. If this name isn't preferred, I am open to suggestion and happy to change it.

…llEditable from DataGrid under certain conditions, breaking edit feature
@PenguinOfWar PenguinOfWar changed the title bug: Renaming isCellEditable utility function bug: name collision in isCellEditable breaks edit feature Oct 5, 2023
@nstepien
Copy link
Contributor

nstepien commented Oct 5, 2023

Have you opened an issue in the Next.js repo?

@PenguinOfWar
Copy link
Contributor Author

PenguinOfWar commented Oct 5, 2023

Have you opened an issue in the Next.js repo?

Logged a bug on next for visibility. vercel/next.js#56480

However as the fix is a minor utility name change in the source (and given the rather large backlog/potentially large turnaround time for the next teams) this felt like the best way to get a quick fix into production and hopefully prevent it from happening on any other bundlers.

@nstepien nstepien merged commit 95489da into adazzle:main Oct 5, 2023
2 checks passed
@nstepien
Copy link
Contributor

nstepien commented Oct 5, 2023

Thanks!

@PenguinOfWar
Copy link
Contributor Author

Thanks for the merge @nstepien!

adityatoshniwal pushed a commit to pgadmin-org/react-data-grid that referenced this pull request Jul 31, 2024
…azzle#3356)

Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
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.

2 participants