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.

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

always decycle objects before cloning #977

Merged
merged 1 commit into from
Jul 27, 2017
Merged

Conversation

jifeon
Copy link
Contributor

@jifeon jifeon commented Jan 26, 2017

I guess it fixes #862, #474 and #914

@ngocketit
Copy link

When is it gonna be merged?

@jifeon
Copy link
Contributor Author

jifeon commented Feb 2, 2017

@ngocketit previous PR about this problem hadn't been being merged for 9 months. You can use @jifeon/winston#2.3.1, it contains only this commit on the top of winston#2.3.1

@ngocketit
Copy link

Thank you @jifeon! However, I couldn't seem to find the repos you mentioned. Could you please help?

@jifeon
Copy link
Contributor Author

jifeon commented Feb 2, 2017

@ngocketit Repo is my fork: https://github.com/plyo/winston
It's published using npm namespace https://www.npmjs.com/package/@jifeon/winston
You can install it with

$ npm i --save @jifeon/winston

and use it like this:

import winston from '@jifeon/winston'

until this PR is merged. Then you can switch back to origin package.

@ngocketit
Copy link

I got error @jifeon/winston' is not in the npm registry but I could install it from your git repos. Thank you!

@jeesus
Copy link

jeesus commented Apr 10, 2017

Any updates?

@vbardales
Copy link

👍

@indexzero indexzero merged commit 9a57be3 into winstonjs:master Jul 27, 2017
@indexzero
Copy link
Member

Thank you for your contribution. Will try to get this out soon.

@jifeon
Copy link
Contributor Author

jifeon commented Jul 27, 2017

Will try to get this out soon.

So cool, it would be nice if you post npm version with the fix here

@david-unergie
Copy link
Contributor

david-unergie commented Aug 31, 2017

Hi,

as we discussed with @crabicode, this pull request seems to bring a regression about Error object.
Meta is {} when error is passed to the formatter func.

winston = require('winston')
logger = new (winston.Logger)({
    transports: [
        new (winston.transports.Console)({
            json: false,
            formatter: (...argv) => {
                console.log(argv);
                return 'baz';
            },
        }),
    ]
})

logger.info('foo', new Error('bar'))

I got on the console:

[ { colorize: false,
    json: false,
    level: 'info',
    message: 'foo',
    meta: {},
    stringify: undefined,
    timestamp: false,
    showLevel: true,
    prettyPrint: false,
    raw: false,
    label: null,
    logstash: false,
    depth: null,
    formatter: [Function: formatter],
    align: false,
    humanReadableUnhandledException: false } ]
baz

@igrek8
Copy link

igrek8 commented Sep 1, 2017

@indexzero, can we decide on further actions on merges, which brought that unwanted behaviour?

@netpoetica
Copy link

I'm trying to determine if this, or an alternative fix, ever got merged in and at which version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RangeError: Maximum call stack size exceeded with customize formatter
8 participants