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

Incorrect docs about importing moment in the example #452

Closed
niieani opened this issue Jul 14, 2016 · 5 comments · Fixed by #453
Closed

Incorrect docs about importing moment in the example #452

niieani opened this issue Jul 14, 2016 · 5 comments · Fixed by #453

Comments

@niieani
Copy link
Contributor

niieani commented Jul 14, 2016

@guidorice commented on Thu Jul 14 2016

I noticed this with skeleton-navigation-1.0.0-beta.3.0.1. Here are the repro steps

cd skeleton-navigation-1.0.0-beta.3.0.1
cd skeleton-typescript-webpack
npm install
npm start # confirmed app is working
npm install moment --save
# Then in app.ts added this:
import moment from 'moment';
# from aurelia docs http://aurelia.io/hub.html#/doc/article/aurelia/binding/latest/binding-value-converters/3
npm start

But now the build fails with
Module '".../node_modules/moment/moment"' has no default export.

This does however, work fine with skeleton-esnext-webpack, so it might be something wrong with the setup of the skeleton-typescript-webpack? Or I am not understanding how to import 3rd party party Js modules into this skeleton.


@EisenbergEffect commented on Thu Jul 14 2016

@niieani Ping


@niieani commented on Thu Jul 14 2016

You'll need to install typings for moment.


@guidorice commented on Thu Jul 14 2016

@EisenbergEffect @niieani

Guys, Please Forgive my ignorance... but it seems there is a gap between the aurelia documentation and the skeleton, which is what I'm trying to point out. I guess I could have worded the issue better.

typings install moment gives this warning which doesn't sound promising:

typings WARN hastypings Typings for "moment" already exist in "node_modules\moment\moment.d.ts". You should let TypeScript resolve the packaged typings and uninstall the copy installed by Typings

Then it fails with some other messages saying it's missing but it is in the dt repository.

Also what if I want to use a library that doesn't have typings available? For example with the numeral library, also suggested in the same documentation link above.

typings install numeral gives this error:

typings ERR! message Unable to find "numeral" ("npm") in the registry.`

Normally with webpack I can just do var foo = require('module'); and webpack figures it out. But that functionality seems to be different here. Maybe because of TypeScript? Or maybe because of the easy-webpack configuration being used in this skeleton?

Suppose I have a commonjs module with no typings available. How would I load it, using the webpack config included with skeleton-typescript-webpack?


@niieani commented on Thu Jul 14 2016

Ok, so it seems moment has built-in typings now, in that case you don't need to install external typings for it!

Looks to me that the way you're importing it (and the way it is presented in the docs) is incorrect. It should be:

import * as moment from 'moment';

We'll need to correct the docs. Sorry for closing too hastily.

@guidorice
Copy link

guidorice commented Jul 14, 2016

Cool, thanks for looking into this. It is a change in the import syntax for numeral and moment modules, and also the usage in date-format.ts and currency-format.ts. They currently fail at runtime with errors:

aurelia-logging-console.js:54 ERROR [app-router] TypeError: moment is not a function(…)

aurelia-logging-console.js:54 ERROR [app-router] TypeError: numeral is not a function(…)

@EisenbergEffect
Copy link
Contributor

I think it's definitely an issue of where Babel has some special code that accounts for both usages, though it's not correct, while TS accounts only for the correct usage. So, the incorrect usage slipped into the docs because it's supported by Babel.

@niieani
Copy link
Contributor Author

niieani commented Jul 14, 2016

@EisenbergEffect Yes, exactly. I believe this feature is configurable via Babel presets.

@guidorice
Copy link

My bug report/issue was with the latest skeleton-typescript-webpack. Isn't TypeScript the transpiler, not Babel? I am so confused!

@niieani
Copy link
Contributor Author

niieani commented Jul 15, 2016

@guidorice Indeed, we mangled two separate issues a bit. This one is about docs, which import moment incorrectly. The fix for docs is here: niieani@3c82331. Finally to end all confusion, the correct way to import (both in TypeScript and ES2015) is to:

import * as moment from 'moment';

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

Successfully merging a pull request may close this issue.

3 participants