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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make the nestedKey only take effect in the serialized object and fix … #885

Merged
merged 7 commits into from
May 19, 2021
Merged

Make the nestedKey only take effect in the serialized object and fix … #885

merged 7 commits into from
May 19, 2021

Conversation

mihai1voicescu
Copy link
Contributor

…error detection and serialization (closes #883)

proto.js

  • Copy the message property in case the obj is an error and there is a msg specified
  • Load the msg as a property of obj if metadata is needed

symbols.js

  • Add nestedKeyStrSym to quickly cache the string

tools.js

  • Manually build the stringify for performance reasons (not too pretty...)

error.test.js + serializers.test.js

  • Add a test for the new nestedKey interaction

Make the nestedKey only take effect in the serialized object and fix …
8443c74 
…error detection and serialization (closes pinojs#883)

proto.js
* Copy the message property in case the obj is an error and there is a msg specified
* Load the msg as a property of obj if metadata is needed

symbols.js
* Add nestedKeyStrSym to quickly cache the string

tools.js
* Manually build the stringify for performance reasons (not too pretty...)

error.test.js + serializers.test.js
* Add a test for the new nestedKey interaction
@mihai1voicescu
Copy link
Contributor Author

I finally managed to make the http test run on local... new antivirus :)

I will fix the coverage ASAP.

mcollina
mcollina reviewed Aug 4, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

From the cloud run it seems there are some regression on the benchmarks.

lib/tools.js Show resolved Hide resolved
jsumners
jsumners previously requested changes Aug 11, 2020
View reviewed changes
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

I think this PR is addressing multiple things at once. If that is the case, please reduce it down to one issue and open subsequent PRs for other issues.

lib/proto.js Outdated Show resolved Hide resolved
lib/proto.js Outdated Show resolved Hide resolved
lib/tools.js Show resolved Hide resolved
@mihai1voicescu
Copy link
Contributor Author

I addressed the issues present in the PR comments.
Note that I solved #885 (comment) by removing the if which causes the obj to miss the msg property. This is available in this.lastMsg. If you are against this I can put it back in.

jsumners
jsumners reviewed Aug 17, 2020
View reviewed changes
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

I don't have time to work through this right now. @mcollina and @davidmarkclements are you able to review?

@mcollina mcollina added the v7 label Oct 22, 2020
mcollina
mcollina approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm, benchmarks are ok

test/metadata.test.js
@@ -40,7 +40,7 @@ test('child loggers works', async ({ ok, same, is }) => {
is(child, this.lastLogger)
is(30, this.lastLevel)
is('a msg', this.lastMsg)
same(this.lastObj, { from: 'child', msg: 'a msg' })
same(this.lastObj, { from: 'child' })
Copy link
Member

Choose a reason for hiding this comment

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

I have a feeling this might be a semver-major change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, should I update the version in the package.json?

@mcollina mcollina added this to the v7.0.0 milestone Oct 22, 2020
@jsumners
Copy link
Member

My last review question never got answered. Is this PR solving multiple issues at once?

@mihai1voicescu
Copy link
Contributor Author

@jsumners it is now addressing a single issue (the nested property).
After your comments I split the other stuff in a different issue.

@mcollina mcollina changed the base branch from master to next October 28, 2020 15:41
@mcollina
Copy link
Member

@jsumners are you ok in landing this?

@jsumners
Copy link
Member

@mcollina sorry, I'm having a really hard time following this and just don't have enough time to devote to it right now. I leave it up to you and @davidmarkclements.

@mcollina
Copy link
Member

Can you please rebase it? I'm sorry we've let this stale.

mcollina
mcollina approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mihai1voicescu
Copy link
Contributor Author

@mcollina looks like one check entitled CI / 12 macOS-latest (pull_request) failed but I don't see any logs associated with it.

What should I do?

@jsumners
Copy link
Member

GitHub Actions is having issues at this time https://www.githubstatus.com/incidents/m16jzl31gnqt

@mcollina mcollina merged commit 75a54cf into pinojs:next May 19, 2021
@mihai1voicescu mihai1voicescu mentioned this pull request May 19, 2021
Closed
@simoneb simoneb mentioned this pull request Oct 21, 2021
Closed
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@markstos markstos markstos left review comments

@mcollina mcollina mcollina approved these changes

@jsumners jsumners Awaiting requested review from jsumners

Assignees
No one assigned
Labels
v7
Projects
None yet
Milestone
v7.0.0
Development

Successfully merging this pull request may close these issues.

Properly handling the 'nestedKey' option
4 participants
@mihai1voicescu @jsumners @mcollina @markstos