Skip to content
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

Using Twix.js with Moment timezone via Webpack #102

Open
l2aelba opened this issue Oct 11, 2017 · 14 comments
Open

Using Twix.js with Moment timezone via Webpack #102

l2aelba opened this issue Oct 11, 2017 · 14 comments
Labels

Comments

@l2aelba
Copy link

l2aelba commented Oct 11, 2017

❗ 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

🎉 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.

@l2aelba l2aelba changed the title Using Twixjs with Moment timezone via Webpack Using Twix.js with Moment timezone via Webpack Oct 11, 2017
@icambron
Copy link
Owner

icambron commented Oct 11, 2017

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
Copy link
Owner

icambron commented Nov 4, 2017

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

@icambron icambron closed this as completed Nov 4, 2017
@baldmountain
Copy link

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 icambron reopened this Nov 15, 2017
@icambron
Copy link
Owner

icambron commented Nov 15, 2017

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
Copy link

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
Copy link

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
Copy link
Owner

icambron commented Nov 15, 2017

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
Copy link
Author

l2aelba commented Nov 16, 2017

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

@DerGuteWolf
Copy link

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
Copy link
Owner

@DerGuteWolf Can you explain that more?

@DerGuteWolf
Copy link

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
Copy link
Owner

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

@DerGuteWolf
Copy link

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
Copy link

gsuess commented Feb 14, 2019

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.

Mihailoff added a commit to Mihailoff/twix.js that referenced this issue Jun 28, 2020
In a multi-plugin world `require 'moment'` leads to problems discussed in icambron#102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants