This repository has been archived by the owner on Dec 31, 2020. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, if you import
moment-holiday
and build your project with Webpack,moment
does not get properly resolved. As such, when you run the build, it will throw at runtime withAlthough Webpack is able to generate the build, it reports critical dependency warnings because
require
is not used correctly:This PR fixes
moment
imports to make them compatible with Webpack. The IIFE syntax I used is based on Rollup (seerollup
repl) and it supports CommonJS, ES Modules, AMD, and UMD formats. You can stillrequire
the module as before, except now you can alsoimport
it.To fix critical dependency warnings, I had to change the way we resolve locales:
__dirname
because it will vary depending on the environment and/or build tool (e.g. inwebpack
it resolves to/
by default)require('./locale/' + locale)
was only used for tests, I wrapped it in aNODE_ENV == 'test'
instead (unsafe equality to keep up with the current code style)require
toeval('require')
- this way Webpack will only resolverequire('../locale/' + locale)
and ignoreeval('require')('./locale/' + locale)
(you can also see this pattern used in Prettier for example).This has an important implication for locales however. Webpack will bundle all locales listed under
/locale
directory. This works if you want to allow the user to callmoment.modifyHolidays.set
with any locale. If not,require('../locale/' + locale)
can be changed toeval('require')('../locale/' + locale)
so it only runs in Node. I'm not sure what the intention here is, so for now I kept it as is.Finally, I found it strange that several builds in
1.5.1
version were missing ingulpfile
, so I added them there to be consistent. Once again, I think we should re-consider which locales get distributed by default. Maybe a topic for another PR :-)P.S. I tested the new builds using
npm link
with (1) webpackimport
(bothweb
andnode
targets), (2) node.jsrequire
, and (3) browser UMD/global, so this should cover backwards compat.Closes #19