winstonjs / winston-daily-rotate-file

A transport for winston which logs to a rotating file each day.
MIT License
899 stars 156 forks source link

4.5.3 requires github access to install... #314

Closed kstott closed 3 years ago

kstott commented 3 years ago

Which broke my build. Was this intended? 4.5.2 still works fine.

PeterOlsenTrifork commented 3 years ago

image

Is there no approval process on this repo? How on earth did this get merged into master?

Klaitos commented 3 years ago

Same here. We had to add RUN apk add git before npm install in our Dockerfile

rlio commented 3 years ago

Same here. We need to install git now

paolominel commented 3 years ago

Same here, please update

mattberther commented 3 years ago

This was needed because winston-transport has not been updated at npm. See https://github.com/winstonjs/winston-daily-rotate-file/issues/313#issuecomment-830844465. Meanwhile, you could pin your package.json to lock winston-daily-rotate-file@4.5.2. I'll update this issue once the winston-transport has pushed a new version to npm.

adavis26 commented 3 years ago

Same here. Had to add git.

stage88 commented 3 years ago

This was needed because winston-transport has not been updated at npm. See #313 (comment). Meanwhile, you could pin your package.json to lock winston-daily-rotate-file@4.5.2. I'll update this issue once the winston-transport has pushed a new version to npm.

I really appreciate the effort to fix TS typings, however I am wondering if this could be done in some other way that does not break builds for existing users. I am more than happy to reach out to maintainers of https://github.com/winstonjs/winston-transport to resolve this issue.

mattberther commented 3 years ago

@stage88 I created #315 as an alternative approach. Do you believe this satisfactorily addresses both issues? If so, I'll merge and publish a new version of the transport.

stage88 commented 3 years ago

@stage88 I created #315 as an alternative approach. Do you believe this satisfactorily addresses both issues? If so, I'll merge and publish a new version of the transport.

Thanks for acting on this so quickly, I've looked at the issue in https://github.com/winstonjs/winston-daily-rotate-file/issues/297, can we please get @halfalicious to verify? Otherwise, I think your change should fixes both issues.

ngraef commented 3 years ago

@mattberther This can be solved by using a tarball URL from GitHub rather than a git URL in package.json dependencies.

- "winston-transport": "github:winstonjs/winston-transport#868d657"
+ "winston-transport": "https://github.com/winstonjs/winston-transport/archive/868d6577956f82ee0b021b119a4de938c61645f7.tar.gz"
mattberther commented 3 years ago

Thanks @ngraef. I like this better than #315. Deployed as winston-daily-rotate-file@4.5.4.

stage88 commented 3 years ago

Thanks for the update @mattberther, however I'll have to pin to a previous version. I am in a corporate environment which doesn't allow github as a package repository. Main reason is being able to execute npm audit for our deps...

Maciek416 commented 3 years ago

@stage88 we're in the same boat, opened #316

mattberther commented 3 years ago

Ultimately, I ended up using the workaround in #315 and pushed as winston-daily-rotate-file@4.5.5. This should address everyone's needs.

I'll track winston-transport and remove the workaround once a new version is dropped.