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

ReferenceError: moment is not defined (Meteor 1.3) #10

Closed
dylanmcgowan opened this issue Jun 9, 2016 · 20 comments
Closed

ReferenceError: moment is not defined (Meteor 1.3) #10

dylanmcgowan opened this issue Jun 9, 2016 · 20 comments

Comments

@dylanmcgowan
Copy link

Hey remcoder,

With meteor 1.3 the convention is importing dependencies like moment from npm, and not having a global atmosphere package. My file looks like this, and it should. Any way you can accommodate Chronos for 1.3? It's a great package and it would be a shame to not be able to use it with the new style.

import moment from 'moment';
import { Chronos } from 'meteor/remcoder:chronos';

Thanks,
Dylan

@remcoder
Copy link
Owner

remcoder commented Jun 9, 2016

Hi Dylan,

Thanks for reporting. I haven't done much with Meteor 1.3 yet (shame on me!) so that's why this occurs. I will look into it. Do you have a suggested fix by any chance?

cheers

@dylanmcgowan
Copy link
Author

dylanmcgowan commented Jun 9, 2016

Hey man! Thanks for the quick reply. I don't know much about authoring packages, but this section of the meteor guide may be what is needed?

Edit: you may have to put that import in a linked 'main module'

Package.onUse(function(api) {
  api.mainModule('my-package.js');
});

@remcoder
Copy link
Owner

@dylanmcgowan Want to verify this is working before I publish to atmosphere?

@dylanmcgowan
Copy link
Author

@remcoder sure! How would I go about doing that without grabbing it from atmosphere? Sorry I am a n00b when it comes to packages

@remcoder
Copy link
Owner

remcoder commented Jun 10, 2016

To do that, create a folder called meteor-packages somewhere on your machine. Then, from inside that directoty, clone the github repo. And finally set the PACKAGE_DIRS environment setting to the directory you made:
export PACKAGE_DIRS=<path to dir>

When you run meteor again, it should pick up the local package. That also means that if you want to go back to using the version on atmosphere you must remove the local package.

See here also

No worries if it's too much of a hassle.

@remcoder
Copy link
Owner

Actually, I'm still fixing some things due to the upgrade to 1.3 so the build is currently failing. You'll want to wait for me to fix that before you test. Sorry about that.

@remcoder remcoder reopened this Jun 10, 2016
@remcoder
Copy link
Owner

Ah should be better now :-)

@dylanmcgowan
Copy link
Author

dylanmcgowan commented Jun 10, 2016

@remcoder Hey so I have the package running locally, pretty cool how that works. I dont think it's being reactive? I may be wrong. My console isn't throwing errors any more though.

Chronos.liveMoment(this.timeStamp).fromNow();

returns "a few seconds ago" and doesn't live update. this.timeStamp is a Date.now() stored in the db.

Also, how do I disable the PACKAGE_DIRS thing so my versions stay up to date with atmosphere?

@remcoder
Copy link
Owner

You can stop meteor and close the terminal and then start a new terminal window. The PACKAGE_DIRS setting will be gone then.

It's weird that it wouldn't be reactive. Can you try other Chronos methods? I have a local test application where Chronos.liveMoment works reactively just fine.

@remcoder
Copy link
Owner

For testing, Chronos.moment().format('ss') would be better as the output from .fromNow() doesn't change for about a minute.

@dylanmcgowan
Copy link
Author

Hmm i feel super dumb. Sorry for not testing as thorough as I should.. In chromes console, I wrapped the fromNow() in an autorun and it spits out the right time.. Looking at my code i found a spelling mistake with timeStamp instead of timestamp. Such a classic mistake. She's working like a charm :)

@remcoder
Copy link
Owner

Awesome! Thanks for taking the time to verify it. Appreciated :-)

@dylanmcgowan
Copy link
Author

No, thank you @remcoder

@remcoder
Copy link
Owner

I just published v0.4.0 to Atmosphere.

However, I'm not closing this issue b/c I'm running into problems when running unit tests that use Chronos.liveMoment(). This fix might not be that good after all. I'm thinking about removing liveMoment from Chronos, possibly to create a stand-alone package for it. Or maybe I should just add moment as a hard dependency of Chronos, something I've been trying to avoid.

I'll have to think about this..

@mikepaszkiewicz
Copy link

mikepaszkiewicz commented Jun 15, 2016

may have been as of the version bump, but the global moment variable is undefined for us within the scope of liveMoment? any help would be much appreciated, thanks!

// wrapper for moment.js
function liveMoment(/* arguments */) {
  // only reactively re-run liveMoment when moment is available
  if (!moment) return;
  liveUpdate();
  return moment.apply(null, arguments);
}

ways I've tried to call it:

//same error with date obj & plain unix timestamp
expiryStamp = this.flash.expires.stamp || new Date(this.flash.expires.stamp)

Chronos.liveMoment(expiryStamp).fromNow()
>> Uncaught TypeError: Cannot read property 'fromNow' of undefined(…)

Chronos.liveMoment(expiryStamp)
>> undefined

@remcoder
Copy link
Owner

How are you including momentjs? As an atmoshere package or as an npm
package?

On Wed, Jun 15, 2016, 18:44 Mike Paszkiewicz notifications@github.com
wrote:

may have been as of the version bump, but the global moment variable is
undefined for us within the scope of liveMoment? any help would be much
appreciated, thanks!

// wrapper for moment.js
function liveMoment(/* arguments */) {
// only reactively re-run liveMoment when moment is available
if (!moment) return;
liveUpdate();
return moment.apply(null, arguments);
}

ways I've tried to call it:

//same error with date obj & plain unix timestamp
expiryStamp = this.flash.expires.stamp || new Date(this.flash.expires.stamp)
Chronos.liveMoment(expiryStamp).fromNow()

Uncaught TypeError: Cannot read property 'fromNow' of undefined(…)
Chronos.liveMoment(expiryStamp)
undefined


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#10 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAcLUvqVVy_WtVPeGFvwE8N_0T-TL5JTks5qMCv7gaJpZM4IyCXI
.

@mikepaszkiewicz
Copy link

atmosphere, momentjs:moment - versions match. i haven't switched to the import/export pattern, seems like that may have something to do with it?

@remcoder
Copy link
Owner

I think the fix for importing moment broke the atmosphere way of including moment. Any global var named moment is shadowed by this code:

let moment;

// if moment is not installed, fine. We don't require it as a hard dependency
try {
   moment = require('moment');
}
catch(e) {
}

That should be any easy fix but on the other hand I don't like the mess that arises. Chronos should now accomodate for 3 situations, being:

  • momentjs is not installed
  • momentjs is installed as an Atmosphere package
  • momentje is installed as an Npm package

This is of course due to the fact that momentjs is not a hard dependeny of Chronos. But I don't want to depend on a library that is way bigger just for the sake of one convenience function.

I think the proper solution would be to remove liveMoment from Chronos and to possibly create a new package just for a reactive version of moment with both momentjs and Chronos as hard dependencies. Not that pretty but definitely more correct.

One big reason why supporting the 3 situations outlined above is difficult, if not impossible, in the long run is that there doesn't seem to be a good way to write automated tests for that. When defining the test in package.js I'll have to choose if and how to depend on momentjs, meaning that there are always 2 scenarios left untested. This is because including packages as a dependency is something that can be done at runtime.

So I'll try to apply an easy fix asap and I'd appreciate everybody advice / stance on the matter for how to handle the situation in the long run.

@remcoder
Copy link
Owner

remcoder commented Jun 15, 2016

tl;dr weak dependencies are evil! Avoid them at all costs :-P

@remcoder
Copy link
Owner

Fixed for now, although kludgy. It's on Atmosphere as v0.4.2.

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

No branches or pull requests

3 participants