Skip to content

chore: Upgrade to RN 0.49.3 #523

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

Closed
wants to merge 1 commit into from
Closed

chore: Upgrade to RN 0.49.3 #523

wants to merge 1 commit into from

Conversation

machour
Copy link
Member

@machour machour commented Oct 20, 2017

With #407 merged, we can now upgrade to RN 0.49.3 with the following patch.

The issue with moment is that the minified version uses dynamic requires, that RN doesn't support anymore (facebook/react-native#16216 (comment)).

Looking at our use of moment.js, I've found that we're only requiring it to compute a timeago whose words we completely rewrite with our translations. Wouldn't make more sense to drop moment.js in favor of a homemade timeago, directly using our translate() function?

@machour
Copy link
Member Author

machour commented Oct 20, 2017

PS: Here is the changelog for 0.49 : https://github.com/facebook/react-native/releases/tag/v0.49.0
We could remove index.android.js & index.ios.js in favor of a single index.js file.

@lex111
Copy link
Member

lex111 commented Oct 20, 2017

Looking at our use of moment.js, I've found that we're only requiring it to compute a timeago whose words we completely rewrite with our translations. Wouldn't make more sense to drop moment.js in favor of a homemade timeago, directly using our translate() function?

Yes, it is true, we use our translations, and from moment just use a function of outputting the date in relative time. If we use moment, and it provides a function to output the date in this format, why write a similar own function?

@machour
Copy link
Member Author

machour commented Oct 20, 2017

@lex111 just concerned about using a library this big (1.4M ?) to achieve so little

🍺  ~/git-point/node_modules (master)*$ du -sh moment/*
 36K	moment/CHANGELOG.md
4.0K	moment/LICENSE
4.0K	moment/README.md
4.0K	moment/ender.js
568K	moment/locale
1.4M	moment/min
 24K	moment/moment.d.ts
128K	moment/moment.js
4.0K	moment/package.js
4.0K	moment/package.json
992K	moment/src

@lex111
Copy link
Member

lex111 commented Oct 20, 2017

@machour fair remark, but if we refuse locales, use only the library?

@machour
Copy link
Member Author

machour commented Dec 7, 2017

The problem I was facing seems to have been fixed in 0.51.0.
I'll try to upgrade to that version. Keeping this PR open for now as a reminder.

@ocarreterom
Copy link
Contributor

I recomend to use date-fns. It is full modular, light and ES6 friendly :)

@machour
Copy link
Member Author

machour commented Dec 10, 2017

@ocarreterom We'd gladly accept a PR to migrate from moment.js to date-fns if it doesn't brake anything and have a smaller footprint:

screen shot 2017-12-10 at 11 31 49 am

😅

@ocarreterom
Copy link
Contributor

@machour no problem! I do it. 👨‍💻

@machour
Copy link
Member Author

machour commented Dec 10, 2017

@ocarreterom great! 🎉 🎉 🎉
Let us know if you need any help at all.

Opened issue #663 to keep track of this change.

@machour
Copy link
Member Author

machour commented Jan 16, 2018

Will be re-opened with a newer RN version

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

Successfully merging this pull request may close these issues.

3 participants