Skip to content

Context Replacement Plugin #2524

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

Merged
merged 7 commits into from
Dec 16, 2018
Merged

Context Replacement Plugin #2524

merged 7 commits into from
Dec 16, 2018

Conversation

masives
Copy link
Contributor

@masives masives commented Sep 18, 2018

Add example on how to tree shake locales from date-fns.

Add example on how to tree shake locales from date-fns
@jsf-clabot
Copy link

jsf-clabot commented Sep 18, 2018

CLA assistant check
All committers have signed the CLA.

@montogeek
Copy link
Member

Please sign CLA.

@masives
Copy link
Contributor Author

masives commented Sep 18, 2018

I wonder though if it would be usefull to show also how the dynamic import is also happening in the place of use to avoid some possible confusion

@EugeneHlushko
Copy link
Member

Wasn't moment example enough for a developer to figure out how to tree shake in similar cases? I mean after seeing two similar examples future contributors might want to put their use cases into example and we end up maintaining them (after example's lib releases breaking change to their structure and this regex wont fit anymore)

@masives
Copy link
Contributor Author

masives commented Oct 9, 2018

I had really hard time figuring this out as we're not checking against filename but filepath.
moment.js

In moment we have a lot of files named after their locales

const getLocale = locale => require(`moment/locale/${locale}.js`)

While in date-fns it is folder name that we need to check:

const getLocale = locale => require(`date-fns/locale/${locale}/index.js`)

It took me some find to find a solution that would properly solve the issue of packing all locales so I decided to suggest this change to avoid this pain for someone that might be looking for this solution.

If you're afraid of maintaining each time library example maybe giving a link to solution would be a good option? date-fns docs/webpack?

@EugeneHlushko
Copy link
Member

Hi @masives
I think afraid is a little too strong here but yes, there is a concern. The link (https://github.com/date-fns/date-fns/blob/master/docs/webpack.md) looks better to me. If you agree please put it into related section of .md file header (like here https://raw.githubusercontent.com/webpack/webpack.js.org/master/src/content/guides/code-splitting.md) with appropriate title

@masives
Copy link
Contributor Author

masives commented Oct 30, 2018

I apologize for taking so long. @EugeneHlushko idea seems perfect for me, PR has been updated.

Copy link
Member

@EugeneHlushko EugeneHlushko left a comment

Choose a reason for hiding this comment

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

ty!

@EugeneHlushko
Copy link
Member

@montogeek pls review

@montogeek montogeek merged commit 1349ca6 into webpack:master Dec 16, 2018
@montogeek
Copy link
Member

Thanks!

EugeneHlushko added a commit that referenced this pull request Dec 17, 2018
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.

5 participants