-
Notifications
You must be signed in to change notification settings - Fork 697
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
fix: various automagic npm module installation fixes #512
Conversation
Big fan, but I'm pretty unsure about including |
@felixrieseberg I agree that it's a pretty big package to fix an edge case in our code, but if we want to provide a stable experience for users to Are there big performance impacts at this bundle level for loading the local bundle? For reference, As for alternatives, I'm not too familiar with using different parsers, but we could probably shave off 25kb here by using Maybe we can lazy load the module rather than import at the top of the file? |
Its unfortunate you can't lean on the tokenizer that's being used internally by monaco. I played around with this a bit but wasn't able to get access to it. microsoft/monaco-typescript#65 I think the only real impact of bloat here is a small bump in bundle size and load time spent parsing/optimizing the additional javascript. |
b9bc445
to
4c6d449
Compare
4c6d449
to
93c24dd
Compare
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.
Good stuff! I'd only check whether this survives compilation but it looks fantastic otherwise 👏
This PR has 3 bugfixes relating to the way we import
npm
modules. Each is in a separate commit.decomment
module inside a Web Worker thread.Set()
.preload
to the list of editors that we check modules for.Note: this PR adds in a new dependency to the
decomment
module, which uses the Esprima JavaScript parser under the hood to strip any JS code fed in of its comments. I decided to use a library rather than play around with regexes for reliability's sake.decomment
is licensed under MIT, so it should be compatible with Electron Fiddle for licensing purposes.