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 a webpack directive for naming icon chunks #1944

Merged
merged 1 commit into from
May 29, 2019

Conversation

pugnascotia
Copy link
Contributor

This PR adds a webpack directive (i.e. a comment) for naming icon chunks. The effect of this is obesrved in applications that use EUI and bundle through Webpack - instead of icon chunks such as e.g.

233.longhashgoeshereomgitssolong.js

you get:

icon.app-kibana-js.longhashgoeshereomgitssolong.js

It's a small thing, but if a dynamic download were to fail then it might make tracking down the problem fractionally easier.

I think it still works if you drop the file extension from the import(), which means you get a cleaner [request] value, but I thought I'd ask about that rather than just doing, since the .js assets live alongside the .svg assets.

@thompsongl
Copy link
Contributor

Removing .js from the request was fine for dev/docs build, but resulted in module parse failures on a full build (attempting to parse the svg files).

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

I'm good with the changes, keeping the js extension as part of the request.

Wondering about the diff to src-docs/src/i18ntokens.json, though. Is it because comments are preserved now? Not sure what impact (if any) this has on the token changelog

@pugnascotia
Copy link
Contributor Author

I don't know what the role of src-docs/src/i18ntokens.json is - can you or @chandlerprall comment?

@pugnascotia
Copy link
Contributor Author

nudging @chandlerprall

@chandlerprall
Copy link
Contributor

I think it still works if you drop the file extension from the import(), which means you get a cleaner [request] value, but I thought I'd ask about that rather than just doing, since the .js assets live alongside the .svg assets.

The .js extension also limits how/where Webpack looks to process the dynamic import. If we don't limit to js then webpack will [pre]process the svg files as well, which is undesired.

Wondering about the diff to src-docs/src/i18ntokens.json, though. Is it because comments are preserved now? Not sure what impact (if any) this has on the token changelog

No impact (and this is a conflict in the PR, master already has those new values, this PR didn't introduce them). The lines are used only in the docs https://elastic.github.io/eui/#/package/i18n-tokens

@chandlerprall
Copy link
Contributor

btw I love this change

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Changing to approval after @chandlerprall input.

Just make sure the CL entry goes to master after resolving conflicts

@pugnascotia pugnascotia merged commit 7c9208a into elastic:master May 29, 2019
@pugnascotia pugnascotia deleted the icon-webpack-chunk-names branch May 29, 2019 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants