GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: replace moment with dayjs by touchRED · Pull Request #737 · postlight/parser

Conversation

@touchRED

@touchRED touchRED commented Mar 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

#731

Replaces Moment with DayJS, a 2kb library with an almost identical API.

Note:

  • DayJS doesn't ship with an equivalent to Moment's parseFormat method used here. I'm not quite sure how long it could take to recreate this method, and I don't believe the moment-parseformat plugin can function without Moment installed. Thoughts?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
touchRED
fix: replace moment with dayjs
98b8f69
@touchRED
touchRED requested a review from johnholdun March 21, 2023 19:08
@touchRED
touchRED marked this pull request as draft March 21, 2023 19:10
@johnholdun

johnholdun commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Nice! There are tons of examples of real-world timestamps in the fixture files which we can test here as well. I think the best we can aim for here is an equal success rate against all those timestamps to what we were getting with moment.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@touchRED

touchRED commented Mar 21, 2023

Copy link
Copy Markdown
Contributor Author

@johnholdun yep i'm running through all the tests now, realized moment was called in more of those than I expected. More to come!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
johnholdun
johnholdun reviewed Mar 21, 2023
View reviewed changes
Comment thread src/cleaners/date-published.test.js
const datePublished = cleanDatePublished('published: 1/1/2020');

assert.equal(datePublished, moment('1/1/2020', 'MM/DD/YYYY').toISOString());
assert.equal(datePublished, dayjs('1/1/2020', 'M/D/YYYY').toISOString());

@johnholdun johnholdun Mar 21, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I know this is how the old tests were too, but can we change all these expected dates into hard-coded strings? I think that would make the functionality we're testing more clear: we expect strings of varying format and quality, and we always return a string in a specific format.

So this line would become:

assert.equal(datePublished, '2020-01-01T00:00:00.000Z');

I think with that change we could probably remove dayjs as a dependency from this test file entirely.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
touchRED reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@touchRED touchRED Mar 21, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

exactly, i'll have a commit with these changes shortly ✅

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
touchRED
fix: update tests to remove dayjs where possible, update formats
6a5f892
@touchRED
touchRED force-pushed the fix-remove-moment-js branch from 1b4cc52 to 6a5f892 Compare March 27, 2023 19:58
touchRED added 4 commits March 28, 2023 18:11
touchRED
fix: update some extractors to remove unnecessary timezones
8ad9309
touchRED
fix: add UTC timezone to extractors where needed
3c336ef
touchRED
fix: few more extractor updates
c49b5ef
touchRED
fix: update al.com timezone
9cf23f1
@touchRED
touchRED force-pushed the fix-remove-moment-js branch from bdda1bb to 9cf23f1 Compare March 29, 2023 17:30
@touchRED

touchRED commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

@johnholdun clearly the updates we spoke about last week took significantly longer than I expected! Now that test-node is passing I'm going to clean things up, leave comments, etc. Question for you: in order to get test-web to pass, do I need to run yarn release and push up new dist files?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@appinteractive

appinteractive commented Mar 1, 2024

Copy link
Copy Markdown

Hey is this issue still be worked on?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@johnholdun johnholdun johnholdun left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@touchRED @johnholdun @appinteractive
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.