icambron / twix.js

:hourglass::left_right_arrow: A date range plugin for moment.js
https://isaaccambron.com/twix.js/
MIT License
379 stars 54 forks source link

Using Twix.js with Moment timezone via Webpack #102

Open l2aelba opened 6 years ago

l2aelba commented 6 years ago

:heavy_exclamation_mark: This is not possible to use Twix.js with moment-timezone when I importing like this via Webpack

import moment from 'moment-timezone'
import 'twix'

And use something like

moment.twix()

So I got error

Uncaught (in promise) TypeError: __WEBPACK_IMPORTED_MODULE_4_moment_timezone___default.a.twix is not a function

:tada: But this is possible to do if I want to use moment as moment-timezone

import moment from 'moment-timezone'
import _moment from 'moment'
import 'twix'

And use something like

_moment.twix()

Question is, Does it possible to use moment.twix() when moment is moment-timezone ?

PS: All are latest versions Thanks so much.

icambron commented 6 years ago

Have you tried this:

import moment from 'moment';
import 'moment-timezone';
import 'twix';

moment.tz // => real thing
moment.twix // => real thing

I haven't tried that in Webpack but I think it should work.

icambron commented 6 years ago

Closing this because of its age. @ me and I'll reopen

baldmountain commented 6 years ago

Hi, we've run into what I think the issue here is. For a larger project there will often be many versions of moment in the node_modules directory. We often get errors like: Uncaught error: moment.utc(...).twix is not a function since we use moment, moment-timezone and twix. Often the instance of moment that moment-timezone extends is not the same as the one twix extends. The usual workaround is to make sure all the libraries we have control of use the same version of moment, moment-timezone and twix. This usually works if we use npm install but if you use yarn this breaks down and there is no way to get this to work.

It would be helpful to have an twix.extend(moment) function that takes a specific version of moment and extends that. For us that would be the one that require('moment-timezone') returns. This way we only have to worry about what version of moment moment-timezone is extending.

I hope I explained that properly.

icambron commented 6 years ago

That's a perfectly reasonable request, but how would it work, given that Twix already tries to do that on its own? I'm loath to break that for existing users. Perhaps it would get Moment on its own automatically and just additionally expose extend(Moment). If that's not too weird, it does seem workable.

But before we go down that path: I had thought the whole point of using peerDependencies was to avoid this kind of thing. Twix uses that to depend on Moment, but I think the way that's supposed to work is that it's the users job to satisfy the dependency, which is exactly what we want. I'm not even close to an expert on node modules, but maybe the whole problem here is only that moment-timezone does not use a peer dependency on Moment? If so, fixing that seems like the cleanest solution.

baldmountain commented 6 years ago

Yeah, that was what I was thinking. extend() would just extend the version passed. It would still automatically extend the moment it gets to not break existing apps.

moment-timezone actually lists moment as a dependency so it may get it's own version. I did a npm ls moment on an existing project and there are 4 different versions of moment in the node_modules directory with two duplicates for a total of 6 instances of moment.

I did a little more poking based on what I said in the previous paragraph and I think the workaround is don't require moment just moment-timezone so it loads it's own version of moment and then twix. Like this:

var moment = require('moment-timezone');
require('twix');

That way twix is getting the moment that moment-timezone is loading. Best to do this as soon as possible in main.js (or which ever file is the app's entry point.) That way moment, moment-timezone and twix are all resolved by node early.

baldmountain commented 6 years ago

You can probably close this since I think the above is the answer. Maybe add a note in the readme for people using twix and moment-timezone.

icambron commented 6 years ago

I'm going to leave this open while I think about ways to make this cleaner. Also, it looks like the OP was trying to use the code you posted (or its ES6 equivalent) with no luck

l2aelba commented 6 years ago

Thanks @baldmountain @icambron for take a look on this issue 👍

DerGuteWolf commented 6 years ago

You could use the approach from https://github.com/jsmreese/moment-duration-format this works for me with moment-timezone, unfortunatly twix works not with moment-timezone. Could you just likewise export the init function?

icambron commented 6 years ago

@DerGuteWolf Can you explain that more?

DerGuteWolf commented 6 years ago

Well, the problem is, with moment-timezone and npm you shouldn't do require('moment'). moment-duration-format does this:

        try {
            module.exports = factory(require('moment'));
        } catch (e) {
            // If moment is not available, leave the setup up to the user.
            // Like when using moment-timezone or similar moment-based package.
            module.exports = factory;
        }

(factory corresponds to your makeTwix) So I can do

const moment = require('moment-timezone');
const momentDurationFormatSetup = require("moment-duration-format");
momentDurationFormatSetup(moment);

As asimple first step, you could also simply do this: return module.exports = moment?makeTwix(moment):makeTwix(require('moment')); (sorry for JS, I am not so familiar with coffee script syntax) which would at least work as long as the users use 'moment' .

icambron commented 6 years ago

That seems reasonable to me. Would take a pull request.

DerGuteWolf commented 6 years ago

Do you mean the way moment-duration does this or the simple first step? As I am not really familiar with coffee-script, I would leave doing the change and a pull to one of others contributing here...

gsuess commented 5 years ago

I think both moment-timezone and twix should not mutate the constructs provided by another package, in this case moment. Or, more generally, imports should not have any side-effects.

There is no value in being able to do moment.twix() over twix(moment) other than the pure cosmetical value of it, but there are so many reasons why moment.twix() is problematic, such as the situations being described here.