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

[webpack] fix loader query string usage #9497

Merged
merged 7 commits into from
Dec 15, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Dec 15, 2016

Edit @epixa: This is a breaking change for the plugin API since certain uiExport types have been removed. Alternatives to these removed types can be found in this comment.

@kobelb noticed that something was up with our webpack config, and it looks like we ran into a longstanding issue in the core webpack dependency enhanced-resolve.

The resolver alias setup in #6177 prevents the configuration parameters (sent as query strings) to the webpack loaders due to webpack/webpack#1289. That bug is closed because it was fixed, but only in webpack 2 which isn't out of beta yet.

Until webpack 2 is GA, or we can convince the webpack maintainers to do another 0.9.x release of enhanced-resolve, I suggest that we use a fork of enhanced-resolve (and by necessity webpack) that applies webpack/enhanced-resolve#22 so we can get control of our webpack loaders back.

EDIT: I did a quick audit of uiExport types used in supported plugins and identified that the webpack-implementation-leaking uiExport types that were potentially impacted by this work could be removed. This includes the loaders, postLoaders, and modules types.

@spalger spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc review v5.2.0 v6.0.0 labels Dec 15, 2016
Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

There are some additional querystring parameters being set https://github.com/elastic/kibana/blob/master/src/ui/ui_bundler_env.js#L147 not sure if we want these to be 'enabled' with this PR or not.

@@ -1,19 +1,23 @@
import { resolve } from 'path';
import { writeFile } from 'fs';
import { inherits } from 'util';
Copy link
Contributor

Choose a reason for hiding this comment

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

inherits and formatUrl are unused, inherits was unused before your changes but formatUrl was added

return makeLoaderString([
{
name: 'babel',
query: defaults({}, query || {}, babelOptions.webpack)
Copy link
Contributor

Choose a reason for hiding this comment

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

Prior to this PR the babelOptions weren't being used, are we intending to add this functionality in this PR?

Copy link
Contributor Author

@spalger spalger Dec 15, 2016

Choose a reason for hiding this comment

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

Yeah, they were in use before #6177 and turning them on makes the babel config in the browser and server consistent.

{ test: /\.png$/, loader: 'url?limit=10000&name=[path][name].[ext]' },
{ test: /\.(woff|woff2|ttf|eot|svg|ico)(\?|$)/, loader: 'file?name=[path][name].[ext]' },
{ test: /[\/\\]src[\/\\](core_plugins|ui)[\/\\].+\.js$/, loader: `rjs-repack${mapQ}` },
{ test: /\.png$/, loader: 'url' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the limit here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was effectively removed by #6177, and has been "working as intended" ever since, so rather than change the behavior again I decided to just match what we are currently doing. If we decided it's better to have then we can always add it back (preferably in another pr)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that the issue you intended to link to? I am not seeing the correlation.

The limit should bundle assets under the specified limit, as opposed to loading them as external assets. In master, I am seeing all assets being external - as expected when the query strings are being ignored. With this PR I am seeing smaller assets being included in the bundles. Where are we defining the limit now? By default it should have no limit and all assets should be external.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change in #6177 adds a resolver alias function, which triggers this bug. I assumed that the limit and name query parameters weren't being used, but if the build output suggests otherwise then we should put them back

@spalger spalger force-pushed the fix/webpack-loader-query-strings branch from bcf8e1c to b841c91 Compare December 15, 2016 18:07
@kobelb
Copy link
Contributor

kobelb commented Dec 15, 2016

@spalger did you mean for the [timelion] convert uiExports.modules -> webpackShims commit to be in this PR?

@spalger
Copy link
Contributor Author

spalger commented Dec 15, 2016

did you mean for the [timelion] convert uiExports.modules -> webpackShims commit to be in this PR?

I did, but I can do it in a new pr if you prefer. It was to facilitate the cleanup of uiExport types that were broken and would now have new behavior with these changes.

@kobelb
Copy link
Contributor

kobelb commented Dec 15, 2016

That's fine, I was just confused when I saw a huge commit pop in there

@spalger spalger force-pushed the fix/webpack-loader-query-strings branch from 803afec to bcaf446 Compare December 15, 2016 20:07
@@ -0,0 +1,2 @@
require('angular');
require('../../../../node_modules/angular-mocks/angular-mocks.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this isn't just require('angular-mocks')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would point to this file :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid I'm missing how require('angular') works and require('angular-mocks') doesn't, not doubting you, just curious.

@kobelb
Copy link
Contributor

kobelb commented Dec 15, 2016

One small comment/question, otherwise LGTM

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

LGTM - 👍 for reducing the plugins exposure to webpack!

@spalger spalger merged commit cb3219c into elastic:master Dec 15, 2016
elastic-jasper added a commit that referenced this pull request Dec 15, 2016
Backports PR #9497

**Commit 1:**
[webpack] pin to fork with fixed loader aliases

* Original sha: a1e3357
* Authored by spalger <[email protected]> on 2016-12-13T21:57:21Z

**Commit 2:**
[optimizer] upgrade to postcss+autoprefixer

* Original sha: bceecd5
* Authored by spalger <[email protected]> on 2016-12-14T23:57:18Z

**Commit 3:**
[timelion] convert uiExports.modules -> webpackShims

* Original sha: 7e90dc6
* Authored by spalger <[email protected]> on 2016-12-15T17:20:54Z

**Commit 4:**
[uiExports] remove implementation-leaking and unused uiExport types

* Original sha: 10016f3
* Authored by spalger <[email protected]> on 2016-12-15T17:59:04Z

**Commit 5:**
[optimizer] remove unused imports

* Original sha: b841c91
* Authored by spalger <[email protected]> on 2016-12-15T18:01:37Z

**Commit 6:**
[uiBundlerEnv] add a method for exporting global import aliases for special cases

* Original sha: 4969d66
* Authored by spalger <[email protected]> on 2016-12-15T18:38:52Z

**Commit 7:**
Merge branch 'master' of github.com:elastic/kibana into fix/webpack-loader-query-strings

* Original sha: bcaf446
* Authored by spalger <[email protected]> on 2016-12-15T19:45:01Z
@epixa
Copy link
Contributor

epixa commented Dec 16, 2016

@spalger Please pull the webpack and enhance-resolve forks under the elastic org

spalger added a commit that referenced this pull request Dec 16, 2016
Backports PR #9497

**Commit 1:**
[webpack] pin to fork with fixed loader aliases

* Original sha: a1e3357
* Authored by spalger <[email protected]> on 2016-12-13T21:57:21Z

**Commit 2:**
[optimizer] upgrade to postcss+autoprefixer

* Original sha: bceecd5
* Authored by spalger <[email protected]> on 2016-12-14T23:57:18Z

**Commit 3:**
[timelion] convert uiExports.modules -> webpackShims

* Original sha: 7e90dc6
* Authored by spalger <[email protected]> on 2016-12-15T17:20:54Z

**Commit 4:**
[uiExports] remove implementation-leaking and unused uiExport types

* Original sha: 10016f3
* Authored by spalger <[email protected]> on 2016-12-15T17:59:04Z

**Commit 5:**
[optimizer] remove unused imports

* Original sha: b841c91
* Authored by spalger <[email protected]> on 2016-12-15T18:01:37Z

**Commit 6:**
[uiBundlerEnv] add a method for exporting global import aliases for special cases

* Original sha: 4969d66
* Authored by spalger <[email protected]> on 2016-12-15T18:38:52Z

**Commit 7:**
Merge branch 'master' of github.com:elastic/kibana into fix/webpack-loader-query-strings

* Original sha: bcaf446
* Authored by spalger <[email protected]> on 2016-12-15T19:45:01Z
@spalger
Copy link
Contributor Author

spalger commented Dec 16, 2016

@epixa Oops, totally forgot to do that

spalger pushed a commit that referenced this pull request Dec 16, 2016
Backports PR #9497

**Commit 1:**
[webpack] pin to fork with fixed loader aliases

* Original sha: a1e3357
* Authored by spalger <[email protected]> on 2016-12-13T21:57:21Z

**Commit 2:**
[optimizer] upgrade to postcss+autoprefixer

* Original sha: bceecd5
* Authored by spalger <[email protected]> on 2016-12-14T23:57:18Z

**Commit 3:**
[timelion] convert uiExports.modules -> webpackShims

* Original sha: 7e90dc6
* Authored by spalger <[email protected]> on 2016-12-15T17:20:54Z

**Commit 4:**
[uiExports] remove implementation-leaking and unused uiExport types

* Original sha: 10016f3
* Authored by spalger <[email protected]> on 2016-12-15T17:59:04Z

**Commit 5:**
[optimizer] remove unused imports

* Original sha: b841c91
* Authored by spalger <[email protected]> on 2016-12-15T18:01:37Z

**Commit 6:**
[uiBundlerEnv] add a method for exporting global import aliases for special cases

* Original sha: 4969d66
* Authored by spalger <[email protected]> on 2016-12-15T18:38:52Z

**Commit 7:**
Merge branch 'master' of github.com:elastic/kibana into fix/webpack-loader-query-strings

* Original sha: bcaf446
* Authored by spalger <[email protected]> on 2016-12-15T19:45:01Z
@epixa
Copy link
Contributor

epixa commented Dec 20, 2016

@spalger Do you have a proposal for alternatives we can suggest to plugin authors that were previously relying on those uiExports?

@spalger spalger deleted the fix/webpack-loader-query-strings branch December 20, 2016 20:33
@spalger
Copy link
Contributor Author

spalger commented Dec 20, 2016

loaders: you'll need to use inline-loader syntax (ie. require('loader!dep')) or pre-build your files before kibana uses them

postLoaders: This is only accessible from the API, and only exposed to allow the test_bundle to add an instrumentation loader. Plugins should not have been using this to begin with.

modules: use webpackShims

If you're trying to figure out how to replicate specific functionality, feel free to mention it here or ping me on IRC

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Jan 19, 2017
clean up

show spy panel in embed mode

[webpack] fix loader query string usage (elastic#9497)

* [webpack] pin to fork with fixed loader aliases

* [optimizer] upgrade to postcss+autoprefixer

* [timelion] convert uiExports.modules -> webpackShims

* [uiExports] remove implementation-leaking and unused uiExport types

* [optimizer] remove unused imports

* [uiBundlerEnv] add a method for exporting global import aliases for special cases

[dev tools] Hide app link when there are no tools (elastic#9489)

* [dev tools] Hide app link when there are no tools

* [dev tools] Add tests for setting app as hidden

pie chart unhandled error fix (elastic#9422)

Add NoResults and Panel components. (elastic#9516)

* Add NoResults and Panel components.

* Lighten noResults text.

Update ToolBarFooter component to support content on the left side. (elastic#9514)

Fix bug with Button component appearance inside of a ToolBar. (elastic#9526)

Make basic Button hover state the same both in and out of ToolBar. (elastic#9528)

[grunt/eslint] fix precommit linting (elastic#9510)

* [grunt/eslint] fix precommit linting

 - remove use of `minimatch.makeRe()` because it does not support the entire glob syntax
 - log a warning whenever a js file is excluded by the `lintStagedFiles` task
 - eslint globs are relative to the project root, ensure that we check against relative version

* [grunt/eslint] only log warning wtr grunt paths

Add Tabs component. (elastic#9536)

- Fix bugs with Button and CheckBox focused states.
- Fix appearance of cell content in Table.

Disable linting for Tabs component example JS. (elastic#9538)

Set Button component to display: inline-block, to ensure it has the same height when applied to both button elements and anchor tags. (elastic#9541)

fixing metric vis to correctly show scrollbars when overflown (elastic#9481)

Adding Safari 7 support to autoprefixer (elastic#9534)

PhantomJS is using a rather outdated version of WebKit, which requires
various css-prefixes to render correctly. PhantomJS doesn't have a specific
user-agent, and Safari 7 is the closet version of WebKit.

use Stop Editing instead of Preview

Warn the user if they Stop Editing with unsaved changes

- Refresh the dashboard after stop editing so unsaved changes are lost
and no temporary edits will be shown in non-edit mode.

Don't watch the variable on scope, but the config attribute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v5.2.0 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants