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

Remove use of Webpack aliases #6430

Closed
5 tasks
Bargs opened this issue Mar 4, 2016 · 8 comments
Closed
5 tasks

Remove use of Webpack aliases #6430

Bargs opened this issue Mar 4, 2016 · 8 comments
Labels
chore dev stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team

Comments

@Bargs
Copy link
Contributor

Bargs commented Mar 4, 2016

One of the benefits of the ES6 import syntax is that it enables static analysis. Our use of webpack aliases limits the benefit by making some import paths dynamic.

I don't know what all the ramifications of this change would be, but I wanted to float the idea of removing our use of aliases.

This can be split up into a number of incremental steps:

  • Replace 'ui' in import declarations with the relative path to src/ui/public
  • Replace imports that rely on webpack-directory-name-as-main with explicit paths to the imported file
  • Replace plugins/${plugin.id} imports with ${plugin.directory}/public
  • Repeat the same 3 steps above for require expressions (is this worth the effort? require won't gain the same static analysis benefits that import statements do, but it'll be necessary if we want to remove the aliases entirely)
  • Remove the aliases? (I'm not 100% if the steps above cover all the prerequisites for deleting the aliases. Can plugins rely on the aliases? That could make removing them more complicated).
@Bargs
Copy link
Contributor Author

Bargs commented Mar 4, 2016

@spalger @epixa I imagine you two have opinions about this.

@epixa
Copy link
Contributor

epixa commented Mar 4, 2016

I couldn't possibly agree more with this, and I have nothing more to add.

+1

@cjcenizal cjcenizal added the Team:Operations Team label for Operations Team label Dec 12, 2016
@Bargs Bargs added chore dev Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc and removed discuss labels Dec 19, 2016
@w33ble
Copy link
Contributor

w33ble commented Dec 19, 2016

I'm generally in favor of this change, but I do want to point out that we need to figure out how we expose functionality from core to plugins, or even expose functionality between plugins outside of core.

One proposal is to follow what we do with on the server code with the server object, were shared functionality is exposed on that object and made available externally.

@stacey-gammon
Copy link
Contributor

For the first one Replace 'ui' in import declarations with the relative path to src/ui/public -- how would that work for external plugins referencing code in there?

Any ideas on if there is a way to achieve this without requiring all the import code to change to a slew of ../../../ which I can see happening if all the paths have to be relative? Would make moving files around a PIA.

@kimjoar
Copy link
Contributor

kimjoar commented Apr 28, 2017

@stacey-gammon Options that I see:

  1. Move the UI code as close to plugins as possible to make the ../.. as short as possible at least (so we might need to restructure a bit).
  2. The only way to get rid of the relative require is to move the code into node_modules in some way, e.g. extracting the relevant UI code into its own packages thing etc. Not sure how nice this would be, but it would let us have absolute requires at least.
  3. Otherwise I see no other option until we bring in the platform changes New Kibana platform #9675 (as you then won't be able to require into Kibana anymore, you'll receive everything you need). However, won't happen for 6.0, so if we want something soon, I think either 1 or 2 are the options we have, unless someone else has creative ideas.

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Apr 28, 2017

Looks like eslint-plugin-import has an ability to write custom resolvers to handle this kind of thing: https://github.com/benmosher/eslint-plugin-import/blob/master/resolvers/README.md

There is also one for webpack: https://www.npmjs.com/package/eslint-import-resolver-webpack

But I'm not knowledgable enough with our webpack set up to figure that one out. For instance, I can't find a webpack config file (except in the ui_framework, but that's not the one for ui/public).

cc @spalger

@kimjoar
Copy link
Contributor

kimjoar commented Apr 28, 2017

You'll find the webpack code in this folder: https://github.com/elastic/kibana/tree/master/src/optimize

(we don't use the webpack config files right now, but configure it at runtime instead (see https://github.com/elastic/kibana/blob/master/src/optimize/base_optimizer.js#L53). @tylersmalley is re-working some of that, but might be a bit longer term.)

@Bargs
Copy link
Contributor Author

Bargs commented Apr 28, 2017

Any ideas on if there is a way to achieve this without requiring all the import code to change to a slew of ../../../ which I can see happening if all the paths have to be relative? Would make moving files around a PIA.

The opposite is actually true if you are using an editor with refactoring tools. When I move files around in Webstorm it automatically updates the relative paths in any modules importing that file. That's one reason I find these webpack aliases really annoying, they make a lot of nice tooling useless.

The plugin question is a good one, unfortunately I don't have an answer to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore dev stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team
Projects
None yet
Development

No branches or pull requests

7 participants