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

Upgrade app-npm example to latest Webpack version #794

Closed
wants to merge 1 commit into from

Conversation

OsmHydrant
Copy link

What I did:

Details about faulty order workaround

By default the index.html would have contained those JS dependencies: vendor, main and finally all the globalize files like i18n/en. This is a problem, as i18n/en is required to be inserted before main. I'm sure this can to be solved within the globalize-webpack-plugin, but I was unable to understand its internals.

Remaining issue

Switching locale cannot be done by just replacing i18n/en by e.g. i18n/de as this causes the following Webpack error in the browser: Uncaught TypeError: Cannot read property 'call' of undefined

However if you keep the i18n/en dependency and just add another one below, it seems to work as the latter de overrules the former en:

<script type="text/javascript" src="vendor.08cec2a19c5b5399ed89.js"></script>
<script type="text/javascript" src="i18n/en.0153a1841b708266fe75.js"></script>
<script type="text/javascript" src="i18n/de.0153a1841b708266fe75.js"></script>
<script type="text/javascript" src="main.df3833cb62cd7f4b527a.js"></script>

Since this is contradictory behavior to the previous version, I'm sure there must be a better solution within globalize-webpack-plugin.

@jsf-clabot
Copy link

jsf-clabot commented Dec 28, 2017

CLA assistant check
All committers have signed the CLA.

return -1;
}
return 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Does it need to be fixed in the webpack plugin instead?

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is here as a better solution than doing something similar in the template, right? Although, it would add all the locales i18n/[locale].[hash].js. The template's purpose is just to give users a basic idea of what they need to do. Users are probably using a better rendering approach in their apps anyway. :)

Copy link
Member

Choose a reason for hiding this comment

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

Your description has a good summary about this, thanks.

However if you keep the i18n/en dependency and just add another one below, it seems to work as the latter de overrules the former en:

Yeap, this shouldn't happen (i.e., it shouldn't be necessary to keep en too). The test I'd make was to keep template as basic and simple as it was, i.e., to make minimal updates in this example necessary to support webpack 2.

Copy link
Author

Choose a reason for hiding this comment

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

I hope the ordering issue can be solved within the plugin without the need to specify the chunks and ordering here.

"vendor",
"globalize-compiled-data-en",
"main"
]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to define these? This should be created automatically by the plugin, right?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I think the plugin should take care of including the chunks in the right order - I just don't know how to do it

@rxaviers
Copy link
Member

rxaviers commented Mar 9, 2018

Sorry for the long delay reviewing your PR

@rxaviers
Copy link
Member

Thank you @OsmHydrant

@rxaviers rxaviers closed this Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants