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

[bugfix] Fix locale autoload, revert #3344 #3438

Closed
wants to merge 1 commit into from

Conversation

ichernev
Copy link
Contributor

This is a redo of #3344 that doesn't (hopefully) break karma + phantomjs -- #3431.
@ljharb @yiminghe let me know if that works for both of you.

@ljharb
Copy link

ljharb commented Sep 15, 2016

I'm afraid not - with 2.15 + this diff, i'm now getting "SyntaxError: import declarations may only appear at top level of a module". I think this approach is defeating babel-node or webpack, because it's not a static require() call.

@ichernev
Copy link
Contributor Author

How about now?

@ljharb
Copy link

ljharb commented Sep 15, 2016

That seems to do it for me, thanks! Looking forward to the patch release :-D

Copy link

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me!

@Aanal
Copy link

Aanal commented Sep 19, 2016

@ichernev thanks for fixing. Do you know when is the next patch release?

@ichernev
Copy link
Contributor Author

I'll only revert #3344 for now, and release 2.15.1

@ichernev
Copy link
Contributor Author

Merged in 0d67e7e

@ichernev ichernev changed the title Fix locale autoload [bugfix] Fix locale autoload, revert #3344 Sep 21, 2016
@ichernev ichernev closed this Sep 21, 2016
ichernev added a commit that referenced this pull request Sep 21, 2016
[bugfix] Fix locale autoload, revert #3344
ljharb added a commit to react-dates/react-dates that referenced this pull request Sep 21, 2016
@mattjohnsonpint mattjohnsonpint added this to the 2.15.1 milestone Sep 21, 2016
@gfviegas
Copy link

gfviegas commented Oct 6, 2016

import moment from 'moment';
import 'moment/locale/pt-br';

moment.locale('pt-br');
console.log(moment.locale()); // en
console.log(moment.locale('pt-br')); // en
console.log(moment.locale('pt-BR')); // en

Locale still doesnt work for me..
Using moment 2.15.1 in Ionic 2 RC0

@ichernev
Copy link
Contributor Author

ichernev commented Oct 6, 2016

@gfviegas can you print output of moment.locales(). You can put a few prints in the code to figure out is the thing loading or not. Also, if you're loading the locale yourself (and you are) this change is not relevant for you.

@gfviegas
Copy link

gfviegas commented Oct 6, 2016

@ichernev the output is:

["en"]

Am I doing something wrong to import the locale? It stopped working when I upgraded my Ionic to RC 0

@gfviegas
Copy link

Any thoughts on my issue?

@ichernev
Copy link
Contributor Author

@gfviegas your locale is not getting loaded. Put a console.log in it. Or it is, but it can't load moment, so it can't attach itself properly. This is not the most complicated system, and so far you're the only one complaining, so I'm expecting to be something in your setup (that is simple to debug), rather than a bug in moment.

@gfviegas
Copy link

@ichernev thing is I don't know where I'm wrong on importing it. I'm following exactly the same example of the docs. Is my import code right?

@ichernev
Copy link
Contributor Author

I said you need to debug it, I didn't say who is wrong or right, just where I'd expect to see the issue. You need to specify a reproducible environment if you want any external help. Most likely if you try to reproduce it cleanly you'll find the issue. Anyway, you can start by putting a few prints and seeing whether the locale file is downloaded and executed to start with.

@gfviegas
Copy link

gfviegas commented Oct 24, 2016

@ichernev I had to

import 'moment/src/locale/pt-br';

instead of

import 'moment/locale/pt-br';

to make it work. I have no idea why.

MarcoAntonioAG added a commit to MarcoAntonioAG/React-dates that referenced this pull request Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants