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.

Sign up for GitHub

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

Jump to bottom

Add 3 proposals to babel config #6335

Merged
merged 17 commits into from
Jan 15, 2020
Merged

Add 3 proposals to babel config #6335

merged 17 commits into from
Jan 15, 2020

Conversation

pnowak
Copy link
Contributor

@pnowak pnowak commented Oct 11, 2019

Context

I needed to add eslint-plugin-babel and update babel-eslint eslint to work with these three experimental features. And I didn't have time to implement private methods, I only changed our privatePool to private fields.

How has this been tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. Add optional chaining and nullish operator to Babel config #6308

Checklist:

  • My code follows the code style of this project,
  • My change requires a change to the documentation.

@pnowak
Copy link
Contributor Author

pnowak commented Nov 4, 2019 •
edited

We have a blocker - babel/babel-eslint#749 - in private method. So we need to wait until this problem will be resolved.
We could withdrawal from private method and implement the rest of the proposals.

@pnowak pnowak marked this pull request as ready for review November 15, 2019 11:17
@pnowak pnowak requested a review from budnix November 15, 2019 11:27
@pnowak
Copy link
Contributor Author

pnowak commented Nov 15, 2019

@budnix After review I'll remove in copyPaste file every change - it has only demonstrative and tests purpose.

wojciechczerniak
wojciechczerniak reviewed Nov 15, 2019
View reviewed changes
src/plugins/copyPaste/copyPaste.js Outdated

this.#isFragmentSelectionEnabled = settings.fragmentSelection;
const { copyPaste: settings, fragmentSelection } = this.hot.getSettings();
Copy link
Contributor

Choose a reason for hiding this comment

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

Renaming copyPaste to settings is confusing. My first thought was that pasteMode is not in our main settings object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not my code, it is on develop branch.
As I said above, copyPaste is only for only demonstrative purpose. And I'll remove all changes after review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, sorry. Didn't noticed that. I had a highlighted change but you're right, that was develop merge.

I don't think you should remove the changes in copyPaste. New operators should stay as a part of the code base and be a living proof that the babel plugins are working correctly.

Copy link
Member

Choose a reason for hiding this comment

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

I think that you can remove all code not related to this PR before review. The description of this PR is enough descriptive to handle a review. Moreover, I can look into the code with example syntax by analyzing previous commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I'm not the reviewer 🤐 😄 👍

budnix
budnix requested changes Nov 18, 2019
View reviewed changes
Copy link
Member

@budnix budnix left a comment •
edited

Choose a reason for hiding this comment

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

It looks good. I will accept the PR after the code from the CopyPaste (unrelated to this changes) plugin is removed.

Additionally, I've encountered an issue related to a specific version of the JSDoc used to generate our documentation. The current version doesn't support nullish coalescing, it throws an error while parsing right now. The JSDoc has to be upgraded with nodeJS (probably to 8.x).

@budnix budnix assigned pnowak and unassigned budnix Nov 18, 2019
@wojciechczerniak
Copy link
Contributor

@pnowak What's our status on this?

@pnowak
Copy link
Contributor Author

pnowak commented Jan 7, 2020

I'm waiting for @budnix to upgrade the node version in the documentation.

@budnix
Copy link
Member

budnix commented Jan 8, 2020

Ok! I've upgraded node version, and we're ready for this PR. However, it's been a while since the last commit to this branch. Can I ask you @pnowak to check dependencies to see if they need updates?

@pnowak pnowak assigned budnix and unassigned pnowak Jan 9, 2020
@pnowak pnowak requested a review from budnix January 13, 2020 09:23
budnix
budnix requested changes Jan 13, 2020
View reviewed changes
Copy link
Member

@budnix budnix left a comment

Choose a reason for hiding this comment

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

You've updated dependencies but it seems that you forgot to commit package-lock.josn with new versions?

@budnix budnix assigned pnowak and unassigned budnix Jan 13, 2020
@budnix
Copy link
Member

budnix commented Jan 13, 2020

@pnowak Please sync with develop branch, here we have some issues with npm audit. Some of the was fixed on develop branch.

@pnowak pnowak assigned budnix and unassigned pnowak Jan 13, 2020
@pnowak pnowak requested a review from budnix January 13, 2020 14:52
@pnowak
Copy link
Contributor Author

pnowak commented Jan 13, 2020 •
edited

It seems that we can remove both plugins - plugin-proposal-optional-chaining and plugin-proposal-nullish-coalescing-operator - https://babeljs.io/blog/2020/01/11/7.8.0
And update babel version to 7.8.0. Isn't it?

@budnix
Copy link
Member

budnix commented Jan 14, 2020

It seems that we can remove both plugins - plugin-proposal-optional-chaining and plugin-proposal-nullish-coalescing-operator - https://babeljs.io/blog/2020/01/11/7.8.0
And update babel version to 7.8.0. Isn't it?

Sure! Let's do it. It can be necessary to bump other babel-related modules.

@budnix budnix assigned pnowak and unassigned budnix Jan 14, 2020
@pnowak pnowak assigned budnix and unassigned pnowak Jan 14, 2020
@pnowak pnowak merged commit f6de657 into develop Jan 15, 2020
@pnowak pnowak deleted the feature/issue-6308 branch January 15, 2020 10:34
jansiegel pushed a commit that referenced this pull request Feb 3, 2020
@pnowak @jansiegel
Add 3 proposals to babel config (#6335)
9179728 
* Update babel core to 7.8.3 version and the other modules related to them, remove the unneccessary plugins #6308
Jafferwaffer pushed a commit to Jafferwaffer/handsontable that referenced this pull request Jul 31, 2020
@pnowak @Jafferwaffer
Add 3 proposals to babel config (handsontable#6335)
d56635c 
* Update babel core to 7.8.3 version and the other modules related to them, remove the unneccessary plugins handsontable#6308
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojciechczerniak wojciechczerniak wojciechczerniak left review comments

@budnix budnix budnix approved these changes

Assignees

@budnix budnix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pnowak @wojciechczerniak @budnix