Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR removes
babelfromevent-kit.Since we are now in an ES6 compatible environment there's really no need to keep around multi-stage build process' and the extraneous extra dependencies that come with that. And it just makes sense to make an effort to simplify things where we can and start to remove the bloat we've inherited.
The one big concern with doing this though is it breaks ES5 style class support.
Now when creating this PR originally I didn't think it'd be that big of a deal and would just be a pain point of trying to modernize things. But now I'm not so sure.
Looking into it, the files here were originally CoffeeScript until Sep 11, 2018
atom/event-kit#38. Then shortly after decaffing the code the community created an issueatom/atom#18157saying that this change breaks some packages, such aslinter-spell&atom-latex.So then this support was added back via
babelinatom/event-kit#44, and later support was added inatom/event-kit#51to be able to require thelibfile here to get ES6 classes, with the default provided ES5 classes.Since apparently, when a package's code is processed by
babelelsewhere it will use ES5 style classes.So that unfortunately potentially means that my PR here can break numerous packages. Which I didn't realize until I started drafting this PR summary.
So maybe we can use this as a way to open up a dialogue about how we want to handle this. Since in my mind, part of our original mission has been to make things modern here, while breaking as little as possible.
So really we have a few options:
In this case, I'd choose option 3. Especially, since my understanding is that Pulsar itself controls
babelparsing for community packages, so maybe that means we could find a way to use thatbabelparsing to update a community package to use ES6 classes. Or we could potentially even use a codemod to do that selectively, where in a perfect world the codemod could check specifically for requiringDisposablefromatomand modify it to be an ES6 require.But curious what others may think on this front.