-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[WIP] React FC conversion for editor file retry + codemirror update #3230
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
Conversation
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
There was a problem hiding this 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?
There was a problem hiding this comment.
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.
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.
@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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
- 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.
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:
- Have babel-jest configured to transform the emmetio module to commonjs by using "transformIgnorePatterns"
- Specify a babel configuration file for the tests. not sure why it is not automatically being detected but looks like others are experiencing the same thing: babel-jest does not locate babel config when it is in root directory jestjs/jest#8006
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 : )
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading. Please reload this page.
Fixes #issue-number
Changes:
lintMessageandfontSizeto dependency array for useEffectI have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123