Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Fix file imports to enable Webpack support #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alex996
Copy link

@alex996 alex996 commented Nov 30, 2019

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

webpack:///./node_modules/moment-holiday/build/moment-holiday.js?:374
 console.log(moment); moment.fn.holiday = function(holidays, adjust) {
                             ^
TypeError: Cannot read property 'fn' of undefined

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.

Closes #19

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lacking Webpack integration
1 participant