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

add architecture styleguide #10094

Merged
merged 4 commits into from
Feb 6, 2017
Merged

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Jan 27, 2017

Closes #5459

Add an Architecture styleguide to the project, explaining the basic layout of a Kibana application.

@epixa
Copy link
Contributor

epixa commented Jan 27, 2017

We should call this the "application architecture" guide rather than simply the "architecture" guide to help differentiate people building applications and working with the core platform, which is currently a big mess that doesn't at all look like the architecture you're describing here.

@w33ble
Copy link
Contributor Author

w33ble commented Jan 27, 2017

Fair enough, that's mostly the target anyway, and it does apply to core plugins, just not to core.

└── index.js
```

The **public** folder is where you place client-side code for your application. This folder will automatically be mounted at a prefix starting with `plugins` and your application's `id`. For example, if your application's `id` is *utile*, and you were trying to import a module from `public/lib/formatter.js`, you would import modules as `plugins/utile/lib/formatter`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to encourage this style of importing when I think we're trying to get rid of our webpack aliases?

Copy link
Contributor Author

@w33ble w33ble Jan 27, 2017

Choose a reason for hiding this comment

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

Well, that's how things work right now, that's simply how you import client-side code. If that changes, it'll happen in a major version, since it'll break literally everything that already exists. And we'll have to update this, and other docs and tools, when that happens.

I could see an argument being made against talking about webpackShims though...

Copy link
Contributor

Choose a reason for hiding this comment

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

Relative paths are still an option within the plugin though. This advice reads to me like we're encouraging the plugins/<id> pattern everywhere, not just across plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I follow you. As a plugin developer, it's important to understand how this works though. It's how we reference public code from the server, in uiExports and hacks, for example. It's not a pattern that's going to be easy to do away with, so I don't see a reason to discourage it here.

Maybe just mentioning it as a NOTE, kind of like an alternative, so it looks less like we're encouraging it is enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that'd be fine. I think as the plugin system evolves, that note will then turn into a "here's how to access a plugin's public API" section.

@Bargs
Copy link
Contributor

Bargs commented Jan 27, 2017

Looks like a good starting point!

@w33ble
Copy link
Contributor Author

w33ble commented Jan 27, 2017

One question I have; we've generally used "application" and "plugin" interchangeably (or at least I have)... do we want to stick to one or the other here, and maybe even outline what we mean when we say it?

@epixa
Copy link
Contributor

epixa commented Jan 27, 2017

Plugin and application are not the same thing, so I'm all for calling that out here. Applications have links in the sidenav and result in complete changes to topnav and the main content area. They are one possible extension point for the plugin system.

and downplay the webpack alias and shims that are available
@w33ble
Copy link
Contributor Author

w33ble commented Feb 6, 2017

@tsullivan @Bargs can you guys take another look at this and let me know if there's anything else you'd like to see changed?

</dd>

<dt>server</dt>
<dd>Ths folder is where server code belongs. Things like custom routes, data models, things that communicate with Elasticsearch, or any other code that should only be executed on the server should go here.</dd>
Copy link
Member

Choose a reason for hiding this comment

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

Minor typo here. Ths => This

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tsullivan new commit pushed

@Bargs
Copy link
Contributor

Bargs commented Feb 6, 2017

lgtm!

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

@w33ble w33ble merged commit c8c03e5 into elastic:master Feb 6, 2017
w33ble added a commit that referenced this pull request Feb 6, 2017
* add architecture styleguide

* be clearer about application architecture

* define as plugin architecture

and downplay the webpack alias and shims that are available

* fix typo, simplify server description
@w33ble
Copy link
Contributor Author

w33ble commented Feb 6, 2017

5.x: 01cf8ff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants