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

Update CodeMirror to v6 & drop Browserify #907

Merged
merged 3 commits into from
Oct 1, 2022

Conversation

NiedziolkaMichal
Copy link
Member

@NiedziolkaMichal NiedziolkaMichal commented Sep 30, 2022

This PR upgrades CodeMirror to version 6, which seems to be written from scratch. New appearance is pretty much the same, except for some minor differences:

  • Web Assembly looks exactly the same
  • HTML looks exactly the same, except for hex entities:
    OLD:
    image
    NEW:
    image
  • CSS has few differences. @layer shouldn't be highlighted(I posted PR in CodeMirror repo about this already). Dot inside Class Selector is not highlighted, but it would require replacing parser. Now also more functions & keywords are colored, so it's a good change. Keyword "all" in @media shouldn't be blue, but it shares color with "from", "selector" & other things which should be blue. 50% in @Keyframes should also have color, but it's not tagged.
    OLD
    image
    NEW:
    image
  • JavaScript has most changes and unfortunately many of them are for the worse. Values undefined, NaN, Infinity, "arguments" share color with variable names. References to already existing variables share color as variable names. ${} share color with normal parethesis. I will file an issue about them in CodeMirror.
    OLD:
    image
    NEW:
    image

Pretty much all of those changes would require rewritting parser and then maintaining it on BOB repo. Instead I will create PRs in Code Mirror and maybe they will be accepted.

browserify & esmify dependencies are dropped. Please take a look if this code should be removed from webpack.config:

  resolve: {
    fallback: {
      fs: false,
      path: require.resolve("path-browserify"),
    },
  }

I have made whole CodeMirror code be in codemirror.js & codemirror.css.
When #888 is merged, I can make CodeMirror support for it too.

Fixes #851
I am also pretty sure that this PR will fix issue with gutters sometimes collapsing.

@queengooborg queengooborg mentioned this pull request Sep 30, 2022
12 tasks
@queengooborg
Copy link
Collaborator

In regards to the resolve: {} configuration, those were added because of the wabt dependency, so they cannot be removed.

Thanks for taking the initiative on this, this is looking amazing to me!

@queengooborg
Copy link
Collaborator

A little nit: could we replace code-mirror with codemirror? This retains consistency with the existing filenames, and the package name is one word rather than two!

@NiedziolkaMichal
Copy link
Member Author

That's a good idea. It's done.

@queengooborg queengooborg changed the title Update Code Mirror to v6 & drop Browserify Update CodeMirror to v6 & drop Browserify Oct 1, 2022
Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Thank you, this is all LGTM! 🎉

@queengooborg queengooborg merged commit 059aa9a into mdn:main Oct 1, 2022
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.

Migrate to CodeMirror v6
2 participants