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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃彈 Use timeago.js library #32809

Merged
merged 8 commits into from Feb 23, 2021
Merged

馃彈 Use timeago.js library #32809

merged 8 commits into from Feb 23, 2021

Conversation

caroqliu
Copy link
Contributor

@caroqliu caroqliu commented Feb 22, 2021 •
edited

This PR:

  • Migrates amp-timeago 0.1 from using the timeago package in third_party/ to the runtime dependency package
  • Upgrades the version of timeago.js being used by amp-timeago:0.1 from 3.0.0 to 4.0.2
  • In doing so, adds support for 7 more languages (!!!)
  • Resolves AMP timeago extension Persian (Farsi) locale聽#30048 as a consequence of the above

Tasks that will follow up in subsequent PR:

  • Migrate amp-timeago 1.0 to do the same, at which point we can
  • Remove third_party/timeagojs entirely

@caroqliu caroqliu requested a review from rsimha February 22, 2021 21:42
rsimha
rsimha approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Very nice PR. So much cleaner!

extensions/amp-timeago/0.1/locales.js Outdated
* This file registers all necessary locales supported by amp-timeago.
*/

import {format, register} from 'timeago.js/dist/timeago.full.min.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

For this to be included by the AMP build, I'm pretty sure the timeago dependency will need to be added to this list:

amphtml/build-system/compile/sources.js

Lines 40 to 68 in 3d0c39b

'node_modules/dompurify/package.json',
'node_modules/dompurify/dist/purify.es.js',
'node_modules/intersection-observer/package.json',
'node_modules/intersection-observer/intersection-observer.install.js',
'node_modules/resize-observer-polyfill/package.json',
'node_modules/resize-observer-polyfill/ResizeObserver.install.js',
'node_modules/obj-str/package.json',
'node_modules/obj-str/dist/obj-str.mjs',
'node_modules/promise-pjs/package.json',
'node_modules/promise-pjs/promise.mjs',
'node_modules/rrule/dist/es5/rrule.js',
'node_modules/web-animations-js/package.json',
'node_modules/web-animations-js/web-animations.install.js',
'node_modules/web-activities/package.json',
'node_modules/web-activities/activity-ports.js',
'node_modules/@ampproject/animations/package.json',
'node_modules/@ampproject/animations/dist/animations.mjs',
'node_modules/@ampproject/toolbox-cache-url/package.json',
'node_modules/@ampproject/toolbox-cache-url/dist/amp-toolbox-cache-url.esm.js',
'node_modules/@ampproject/viewer-messaging/package.json',
'node_modules/@ampproject/viewer-messaging/messaging.js',
'node_modules/@ampproject/worker-dom/package.json',
'node_modules/@ampproject/worker-dom/dist/amp-production/main.mjs',
'node_modules/preact/package.json',
'node_modules/preact/dist/*.js',
'node_modules/preact/hooks/package.json',
'node_modules/preact/hooks/dist/*.js',
'node_modules/preact/compat/package.json',
'node_modules/preact/compat/dist/*.js',

kristoferbaxter
kristoferbaxter reviewed Feb 22, 2021
View reviewed changes
Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Question about the approach.

extensions/amp-timeago/0.1/locales.js
* @param {string} locale
* @return {string}
*/
export function getLocale(locale) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be doing this in JS or processing the locale information in a publishing step from the origin library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the latter, I believe we would need an I2D on the formats converted here and keep this as an intermediary step. Unless I am misunderstanding what you are suggesting?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion was to remove the mapping here and generate the output from the library output as a build step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh got it, done.

Copy link
Contributor Author

@caroqliu caroqliu Feb 23, 2021 •
edited

Choose a reason for hiding this comment

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

Re-opening this thread. After struggling with adding to sources.js and chatting with @samouri, it seems as though the patch approach during build isn't compatible with importing the minified modules. He recommended in 3cd6067 to keep this in JS to most clearly get gulp dist to build. Maybe we can open a TODO to see if it's possible to move over in a separate effort?

Copy link
Member

@samouri samouri Feb 23, 2021 •
edited

Choose a reason for hiding this comment

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

A couple of things:

  1. It would be nontrivial to point closure compiler to either the esm or lib folders of timeago and have it work correctly. depending on which folder you try to point to, something explodes. (e.g. expecting all the es5 imports to have .default after their import etc). For that reason, using the files within the prebuilt dist folder is the only feasible option unless we move off Closure.
  2. Since our locale format doesn't match up with timeago, we need to have the conversion somewhere. The absolutely ideal location is of course for only a single mapping layer to exist as Kris says, but to patch the minified file would be difficult (file is minified) and brittle (liable to break at any package update) with only a very small bytesize win

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to merge this for now. If folks have any recommended changes from this discussion happy to follow up!

@ampproject ampproject deleted a comment from amp-owners-bot bot Feb 23, 2021
@caroqliu
Copy link
Contributor Author

caroqliu commented Feb 23, 2021 •
edited

Apologies to folks who may have been tagged in this PR by owners bot, I did that thing where I accidentally took other people's commits :) They have since been removed.

rsimha
rsimha approved these changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Glad you got this to work. It's good to see that bundle-size caught the change in binary size and that the change isn't substantial.

image

@caroqliu caroqliu merged commit 96f48cd into ampproject:master Feb 23, 2021
@caroqliu caroqliu mentioned this pull request Feb 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristoferbaxter kristoferbaxter kristoferbaxter left review comments

@samouri samouri samouri left review comments

@rsimha rsimha rsimha approved these changes

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AMP timeago extension Persian (Farsi) locale
5 participants
@caroqliu @kristoferbaxter @samouri @rsimha @ampprojectbot