Skip to content

chore: Upgrade to RN 0.49.3#523

Closed
machour wants to merge 1 commit into
gitpoint:masterfrom
machour:react-native-0.49.3
Closed

chore: Upgrade to RN 0.49.3#523
machour wants to merge 1 commit into
gitpoint:masterfrom
machour:react-native-0.49.3

Conversation

@machour

@machour machour commented Oct 20, 2017

Copy link
Copy Markdown
Member

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 (react/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

machour commented Oct 20, 2017

Copy link
Copy Markdown
Member Author

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

lex111 commented Oct 20, 2017

Copy link
Copy Markdown
Member

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

machour commented Oct 20, 2017

Copy link
Copy Markdown
Member Author

@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

lex111 commented Oct 20, 2017

Copy link
Copy Markdown
Member

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

@machour

machour commented Dec 7, 2017

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Contributor

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

@machour

machour commented Dec 10, 2017

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
Contributor

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

@machour

machour commented Dec 10, 2017

Copy link
Copy Markdown
Member Author

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

Opened issue #663 to keep track of this change.

@machour

machour commented Jan 16, 2018

Copy link
Copy Markdown
Member Author

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