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

Get rid of createLegacyClass #21360

Closed
timroes opened this issue Jul 27, 2018 · 8 comments
Closed

Get rid of createLegacyClass #21360

timroes opened this issue Jul 27, 2018 · 8 comments
Labels
chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@timroes
Copy link
Contributor

timroes commented Jul 27, 2018

We have a way to inherit classes in an old fashioned style in legacy_class. Trying to convert some old Mocha tests to Jest (which we compile differently in Babel, because they run in node.js and not the browser) I found some issues with that class.

createLegacyClass currently only works, because we are compiling specific features (in this case class) down to ES5. Since we are not compiling down to ES5, but use babel target node: 'current' for Jest tests, this class does not work in Jest tests. In case we would ever stop compiling class to ES5 (e.g. we decide to no longer support IE11 or such), this class would also not work in the Kibana distributable anymore.

The cause is, that class Foo {} will lead to Foo.prototype being writable: false from its descriptor (Object.getOwnPropertyDescriptor), while the babel compiled code would contain function Foo {} which makes Foo.prototype writable. createLegacyClass tries to overwrite prototype and thus fails if we are not compiling to ES5.

This needs to be fixed before we ever possibly could get rid of compiling class no longer down for Kibana during runtime.

cc @elastic/kibana-platform

@timroes timroes added bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jul 27, 2018
@mattkime
Copy link
Contributor

Summarizing and verifying -

  1. createLegacyClass is a problem only where its called on a class
  2. This is the primary obstacle in porting a large portion of tests from mocha to jest

@epixa
Copy link
Contributor

epixa commented Nov 30, 2018

I'm pretty sure createLegacyClass can just be replaced with native class...extends, in which case this isn't really a platform thing. Teams need to remove their usage of that utility, then we can remove the utility. A quick search shows it's mostly (maybe all) apps and platform.

If we want to park it in the platform queue for lack of an obvious alternative, we can.

@epixa epixa added chore and removed bug Fixes for quality problems that affect the customer experience labels Nov 30, 2018
@mattkime
Copy link
Contributor

Scope of work - createLegacyClass is used in 20 places. Removal sounds manageable.

@pgayvallet
Copy link
Contributor

Only remaining usages are in the legacy state_management (src/legacy/ui/public/state_management). I guess this would be resolved when we complete legacy removal.

@timroes
Copy link
Contributor Author

timroes commented Jul 6, 2020

cc @flash1293 Do you know if those state_management utils are going away automatically with legacy removal?

@flash1293
Copy link
Contributor

legacy state_management is only used in timelion which will be migrated away from it soon. This can simply be removed along with the legacy platform.

@lukeelmers
Copy link
Member

Pretty sure this has been removed for awhile now -- @flash1293 is this issue safe to close?

@pgayvallet
Copy link
Contributor

createLegacyClass: no occurrence found in master code. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

6 participants