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

Use ICodeMirror, more type imports, order plugin for fixer #576

Merged
merged 7 commits into from
Apr 5, 2021

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Apr 4, 2021

Binder

References

Code changes

  • uses import type for most/all import 'codemirror'
    • should help the compiler/webpack know we don't want another codemirror
  • uses ICodeMirror.CodeMirror for the CMSyntaxHighlighting.get_mode
    • this looked like the only case of actually calling a "dirty" static method
    • however, we may want to hoist codeMirror to the baseline lab_integration...
  • add the import-plugin plugin for eslint to see if everything's good
    • this is way more aggresssive than the default, but at least can fix most of the issues (let it run a few times with tweaks)
    • we lot some comments to sorting
    • collapsed some duplicated imports
    • doesn't like our export class and export namespace pun: i never did much, either, but here we are
    • apparently we have been making lots of cyclical imports... maybe they're just types, but yeah. ~200 errors
      • had to disable for now
    • would like some way to say, only allow importing codemirror with import type, haven't found yet
    • does require building with tsc (and all the schema stuff) before linting...
      • updated jlpm bootstrap as such
  • add a test to verify we aren't shipping codemirror
    • i am not exactly sure how to do this... when it does a dev build, the node_modules__codemirror__codemirror_js shows up... but perhaps there's a way to look at a production remoteEntry and find magic strings...

User-facing changes

  • n/a

Backwards-incompatible changes

  • n/a

Chores

  • linted
  • tested
  • documented
  • changelog entry

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Apr 4, 2021

A heads-up: I am unlikely to finish this work in the next few days, unless everything basically works the first time...

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Apr 4, 2021

I guess re: size: not shipping codemirror probably saves ~150kb, so we could start tracking the wheel/sdist size, and fail builds that increase it above a certain size... I don't see any particular tools out there for doing so, but would be easy enough to add to our test suite, if we don't mind hoisting the hardcoded value to... somewhere. Maybe just expect it in an environment variable, and ignore if not set.

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Apr 4, 2021

Well, looks like some stuff is passing, so it's probably not entirely broken... once I can pull the built artifacts, I'll try to put together a CI check for the artifact size.

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Apr 4, 2021

This critical bit looks good locally (finally):

Screenshot from 2021-04-04 14-21-04

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Apr 4, 2021

Also the sdist artifact size appears about right: 310kb (from pypi) vs 251.5kb (just built).

Unfortunately, neither remoteEntry has an explicit reference to 'codemirror'... still trying to dig up how we might be sure it's correct... package.json also not very helpful...

@@ -25,8 +25,8 @@
"vscode-css-languageserver-bin": "^1.4.0",
"vscode-html-languageserver-bin": "^1.4.0",
"vscode-json-languageserver-bin": "^1.0.1",
"yaml-language-server": "^0.12.0",
"vscode-json-languageservice": "^3.9.1 <3.10.0"
"vscode-json-languageservice": "^3.9.1 <3.10.0",
Copy link
Member

Choose a reason for hiding this comment

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

Do you remember why we have the pin for <3.10.0? It looks like we could update to 3.11.0 or 4.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alas, i do not... it's one of the language servers that might not get updated very often...

Copy link
Member

@krassowski krassowski Apr 4, 2021

Choose a reason for hiding this comment

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

I will try bumping it then, to make sure we don't stay behind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, it was yaml. they are up on 0.17 now, so presumably work with the newer service.

Of course, it is irrelevant to our JSON language server, because it's a hella old static dump of... some snapshot or another. Anyhow, would prefer to not move too many more rocks on this PR... very scared of finding more things.

Comment on lines 9 to 20
BAD_CHUNK_PATTERNS = {
(
"codemirror_codemirror",
"codemirror_lib",
): """
Please ensure is CodeMirror is imported by type, e.g.

import type * as CodeMirror from 'codemirror'

see https://github.com/krassowski/jupyterlab-lsp/issues/575
"""
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Test of not having codemirror_codemirror or codemirror_lib in our dev build is the best I could come up with, seems to be working:

https://github.com/krassowski/jupyterlab-lsp/pull/576/checks?check_run_id=2265592002#step:15:50

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gah, and a typo...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Welp, guess i'll fix that typo and call this done...

@bollwyvl bollwyvl changed the title [wip] use ICodeMirror, more type imports, order plugin for fixer Use ICodeMirror, more type imports, order plugin for fixer Apr 4, 2021
@bollwyvl bollwyvl requested a review from krassowski April 4, 2021 20:55
@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Apr 4, 2021

Yep, tried some more looking, and short of upstreaming something to the eslint plugin, don't see how we can enforce the codemirror issue. The post-dev-build test will have to be good enough for now.

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.

Private CodeMirror shipped, used in a number of places instead of The CodeMirror
2 participants