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 with
Although Webpack is able to generate the build, it reports critical dependency warnings because require is not used correctly:
WARNING in ./node_modules/moment-holiday/build/moment-holiday.js 8:50-57
Critical dependency: require function is used in a way in which dependencies cannot be
statically extracted
@ ./src/index.js
WARNING in ./node_modules/moment-holiday/build/moment-holiday.js 290:8-30
Critical dependency: the request of a dependency is an expression
@ ./src/index.js
This PR fixes moment imports to make them compatible with Webpack. The IIFE syntax I used is based on Rollup (see rollup repl) and it supports CommonJS, ES Modules, AMD, and UMD formats. You can still require the module as before, except now you can also import it.
To fix critical dependency warnings, I had to change the way we resolve locales:
For one, we can't rely on __dirname because it will vary depending on the environment and/or build tool (e.g. in webpack it resolves to / by default)
Since require('./locale/' + locale) was only used for tests, I wrapped it in a NODE_ENV == 'test' instead (unsafe equality to keep up with the current code style)
Lastly, I had to change require to eval('require') - this way Webpack will only resolve require('../locale/' + locale) and ignore eval('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 call moment.modifyHolidays.set with any locale. If not, require('../locale/' + locale) can be changed to eval('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 in gulpfile, 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) webpack import (both web and node targets), (2) node.js require, and (3) browser UMD/global, so this should cover backwards compat.
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