Skip to content

Remove String manipulations of Date objects #268

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 0 commits into from

Conversation

bbito
Copy link
Contributor

@bbito bbito commented Apr 25, 2017

Description

Pull Request requested by @kjdelisle regarding #149 following my proposed solution "A" at #149 (comment).
As mentioned in the post linked above, this change allows the underlying mysqljs/mysql module to handle Date object serialization and removes the forced conversion of Dates to UTC Strings. An opt-out fallback to forced coercion to UTC is included, which was modeled after #265 from @darknos .

Previously timezone values specified in datasources.json had no effect on the underlying mysqljs/mysql module because Loopback was converting the Dates to Strings before handing off to the driver module.
With this change, timezone values specified in datasources.json provide the expected behavior only if "legacyUtcDateProcessing" is explicitly set to false in datasources.json, e.g.
"legacyUtcDateProcessing": false

This PR was made under conditions spelled out in the related issue thread #149 that I have limited time and expertise as a contributor and do not plan to personally address the test creation and documentation outlined in the action plan from @bajtos as pertains to this correction of long-standing undocumented forced coercion of Date types to UTC.

Please label: breaking-change, bug as per #149

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@slnode
Copy link

slnode commented Apr 25, 2017

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Apr 25, 2017

Can one of the admins verify this patch?

2 similar comments
@slnode
Copy link

slnode commented Apr 25, 2017

Can one of the admins verify this patch?

@slnode
Copy link

slnode commented Apr 25, 2017

Can one of the admins verify this patch?

@kjdelisle
Copy link
Contributor

@slnode test please

@kjdelisle kjdelisle added the apex label Apr 25, 2017
@kjdelisle kjdelisle self-assigned this Apr 25, 2017
@kjdelisle kjdelisle requested a review from ssh24 April 25, 2017 14:13
@ssh24
Copy link
Contributor

ssh24 commented Apr 25, 2017

@bbito Could you please rebase your branch and fix the commit message? The format for commit messages:

1. First line of the commit message should be no longer than 50 characters.
2. Second line of the commit message should be empty.
3. Every line after the second line should be no longer than 72 characters long.

@ssh24
Copy link
Contributor

ssh24 commented Apr 25, 2017

@bbito Sorry, do not worry about the rebase. I was able to do that for you :)

@bbito
Copy link
Contributor Author

bbito commented Apr 25, 2017

@ssh24 Oh no!
I was trying to do rebase -i here and there when I had time at work and now I think I've really borked it! I didn't see your message that you were fixing it...

@bbito
Copy link
Contributor Author

bbito commented Apr 25, 2017

@ssh24 is there an easy way I can back-out this mess?

@ssh24
Copy link
Contributor

ssh24 commented Apr 25, 2017

@bbito Let me see if I can fix this.

@bbito
Copy link
Contributor Author

bbito commented Apr 25, 2017

@ssh24 Thanks a bunch! I thought the rebase was going weird - perhaps because we were both doing it at the same time...

@ssh24
Copy link
Contributor

ssh24 commented Apr 25, 2017

@bbito I gave it a try but the commit log looks very messy and quiet impossible to rebase it at this point. I also do not remember your changes hence cherry picking it into new commit is becoming difficult. Do you mind creating a new PR?

@bbito
Copy link
Contributor Author

bbito commented Apr 25, 2017

@ssh24 I'm willing to do it, but I need a little direction! At this point can I make a new branch and bring that to upstream/master, make the edits to mysql.js and then make a single commit?

@ssh24
Copy link
Contributor

ssh24 commented Apr 25, 2017

Yes that should work. Please make sure to rebase against upstream/master and not origin/master in the future until and unless your origin/master is already up-to-date with the upstream/master.

Also as a side note: I suggest community members to not use their master branch to submit PR's. It is good practice to create branches of master and committing work on that. Forked master branch should be used to keep codebase synced with the upstream/master at the very most.

@slnode
Copy link

slnode commented Apr 25, 2017

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Apr 25, 2017

Can one of the admins verify this patch?

2 similar comments
@slnode
Copy link

slnode commented Apr 25, 2017

Can one of the admins verify this patch?

@slnode
Copy link

slnode commented Apr 25, 2017

Can one of the admins verify this patch?

@bbito
Copy link
Contributor Author

bbito commented Apr 25, 2017

Hi @ssh24 - WOW, that rebase attempt really borked my git! I finally made the changes to a fresh branch and put up #271

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.

4 participants