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

Disallow importing Joi client side #55018

Merged
merged 6 commits into from
Jan 24, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jan 16, 2020

Summary

After this blocker: #54888 arose blocking 7.6 QA (and a previous case of joi causing an issue in IE11 (#36493)).

This seems like something an automated check can help with.

How to review

  • test importing joi in x-pack plugin public
  • test importing joi in oss plugin public

Screenshot

Piggy backing off of existing eslint rules

Screenshot 2020-01-16 at 11 12 04

@jloleysens jloleysens added discuss v8.0.0 v7.7.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 16, 2020
@jloleysens
Copy link
Contributor Author

Looks like eslint has picked up two cases of joi inside shared module code. We can escape the eslint rule there?

@flash1293
Copy link
Contributor

flash1293 commented Jan 16, 2020

The first one looks like it is only used on the server, so it should be save to move it to a server directory together with the whole lib folder (the folder structure of this plugin is strange anyway). @joshdover looks like this is a platform concern, is it fine to put lib into a server folder?

The second one will be resolved by #54910 (should be done soon)

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Tested locally, code LGTM. Thanks for adding this, JL!

@joshdover
Copy link
Contributor

The first one looks like it is only used on the server, so it should be save to move it to a server directory together with the whole lib folder (the folder structure of this plugin is strange anyway). @joshdover looks like this is a platform concern, is it fine to put lib into a server folder?

Yep, should be good to move that one.

@LeeDr LeeDr added the IE11 label Jan 22, 2020
@LeeDr
Copy link

LeeDr commented Jan 22, 2020

I'm not sure if we want the v8.0.0 label on this PR. We may be dropping support for IE11 in 8.0.0. If that decision is made, will someone remember to change the eslint rule?

@jloleysens jloleysens requested a review from a team as a code owner January 22, 2020 10:26
@jloleysens
Copy link
Contributor Author

@LeeDr that makes sense to me! I've also added a TODO to the lint rule.

@jloleysens jloleysens removed the v8.0.0 label Jan 22, 2020
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

LGTM

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jloleysens jloleysens merged commit f7c53b5 into elastic:master Jan 24, 2020
@jloleysens jloleysens deleted the no-joi-eslint-rule branch January 24, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss IE11 release_note:skip Skip the PR/issue when compiling release notes v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants