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

[WIP] React FC conversion for editor file retry + codemirror update #3230

Draft
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

nahbee10
Copy link
Collaborator

@nahbee10 nahbee10 commented Sep 10, 2024

Fixes #issue-number

Changes:

  • using EditorView along with editorView as a react state instead of cmInstance.current
  • using view.dispatch when changes need to be applied to the view instead of calling the function directly from the instance
  • change linter/emmet usage style to accommodate the updates from codemirror 6
  • will work on creating separate hook(useCodemirror) to set up the codemirror view to clean up the code
  • add mobile editor
  • apply p5 theme color
  • TODO: add custom color picker
  • TODO: implement emmet
  • TODO: connect/implement search
  • TODO: refactor the code into smaller chunks
  • TODO: add back the debouncer
  • TODO: add lintMessage and fontSize to dependency array for useEffect
  • TODO: fix style discrepancy issue on each theme

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@nahbee10 nahbee10 marked this pull request as draft September 10, 2024 00:55
Copy link

release-com bot commented Sep 10, 2024

Release Environments

This Environment is provided by Release, learn more!
To see the status of the Environment click on Environment Status below.

🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-fdb9d83e1a

Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that you closed my old draft PR #2367. It was not 100% done but there is a lot of good code in there that I hope you can use, particularly with all of the extensions and customizations.

You have to be very careful that any function which you provide to the CodeMirror editor will be called with the right variables at the time of execution. The state management part of working with CodeMirror is a nightmare because it has its own internal state which is handled very differently from React. It looks like the @uiw/react-codemirror essentially turns the editor into a controlled component so that could help. If you want I can update my WIP to use the @uiw/react-codemirror package. I'm obviously biased but I think that would make a lot more sense than starting over from scratch since I already tackled a lot of our modifications/enhancements such as the custom search box.

@raclim I wrote 2,000 lines of code which just sat there for over a year with zero feedback. Did anyone even read the code? Or check out and run the branch? Then after a year it just gets unceremoniously closed. Still zero feedback on anything that I wrote. And I see no semblance of a plan of how this conversion will be handled. Is @nahbee10 working on it? Is it a priority?

closeOnUnfocus: false
view.dom.style.fontSize = `${fontSize}px`;

view.dom.addEventListener('keydown', onKeyDown);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any interaction with the DOM should be inside of a useEffect, where your cleanup function removes the previous listener when adding a new or updated one. Otherwise you will be adding an additional event handler on every render.

@nahbee10
Copy link
Collaborator Author

nahbee10 commented Oct 5, 2024

Hi @lindapaiste Glad to meet you since I was also closely looking at your code when I'm preparing to work on this feature.
First I apologize that since I didn't mean to close your PR. @raclim and I talked about closing my other old editor conversion/codemirror update PR yesterday and I accidentally closed yours. Now I reopened it. So sorry about that!
And thanks for reviewing the PR. I've been working on codemirror upgrade as a part of the pr05 program with updating the p5 editor codebase using React functional component. While i'm working on that, I also realized that it needs a lot care for state management and custom components handling missing/changed features from the upgrade. Had a better understanding about how much effort you put in the draft PR after I actually worked on upgrading the codebase. I'll address the comments you left and consider carefully the amount of works need to be done for building custom features.

@lindapaiste
Copy link
Collaborator

@nahbee10 I'd guess that I have it around 70% done in that other PR (so you can imagine my frustration). I would recommend that you make a checklist with all of the behaviors and styles which need to be re-implemented when upgrading to v6. In particular: all of the addons that are imported, the addons that we uploaded and modified, the options that we set, and the keyboard shortcuts. Then you can run the branch for my draft and see which ones I have and have not addressed. I left a bunch of //TODO comments that you can look at to see what needs to be done.

package.json Outdated
"@gatsbyjs/webpack-hot-middleware": "^2.25.3",
"@pmmmwh/react-refresh-webpack-plugin": "^0.5.11",
"@redux-devtools/core": "^3.11.0",
"@redux-devtools/dock-monitor": "^3.0.1",
"@redux-devtools/log-monitor": "^4.0.2",
"@reduxjs/toolkit": "^1.9.3",
"@replit/codemirror-css-color-picker": "^6.2.0",
"@simonwep/pickr": "^1.9.1",
"@uiw/react-codemirror": "^4.23.1",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're not using this one let's remove it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed most but left this @replit/codemirror-css-color-picker to see if it is useful to implement the color picker!


const replaceCommand = metaKey === 'Ctrl' ? `Mod-h` : `Mod-Alt-f`;

// Handle document changes (debounced)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't see this being debounced? unless i missed something (very likely haha)? but if it's not being debounced lets change the comment or if it should be, let's add the debouncer

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah thanks for picking this up! I think i need to add back the debouncer. Will update it on the next commit!

nahbee10 and others added 5 commits October 9, 2024 22:33
- Upgrades jest version to 29.7
- We were getting errors related to the new emmetio import. This is because emmetio formerly compiled to both commonJS and ES modules but now only compileds to ES modules. By default, Babel will ignore node_modules, but Jest does not support ES modules out of the box (only commonJS), so we add a custom transformIgnorePatterns so that Babel knows to transform emmetio to commonJS in the jest environment.
@khanniie
Copy link
Collaborator

khanniie commented Oct 24, 2024

Just wanted to provide a little more info about the test fixes in case anyone is curious!

The jest/babel changes are meant to address this error that we were formerly getting:

-  Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

The problematic import in question was the newly introduced emmetio/codemirror6-plugin, which only compiles to a ECMAScript module, which is not enabled by default in Jest, as specified in the above error message.

What we do to fix this is:

I also updated Jest in case that helped since it looks like a lot of new features had been introduced.

The other changes in the same CL are just small test error fixes but nothing interesting : )

@nahbee10 nahbee10 changed the title [WIP] React FC conversion for editor file retry + codemirror update using uiw/react-codemirrror [WIP] React FC conversion for editor file retry + codemirror update Oct 25, 2024
} from '@codemirror/search';

import classNames from 'classnames';
import StackTrace from 'stacktrace-js';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't necessarily have to be addressed in this PR, but wanted to note that stacktrace-js is pretty outdated now should probably be replaced! Potentially with maybe strack-trace?

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.

4 participants