feat: add logging facilities #486

tlaundal · 2023-01-16T19:01:37Z

~~Uses the context.warn function to issue errors from the transformers and prints these from the vite and rollup plugins. If the silent option is set, the warnings are not printed.~~

Logs messages through Rollup and Vite so message verbosity can be tuned.

I have tested this in a Vite project, but had no Rollup project to test in.

Quick Checklist

I have read the contributing guidelines
I have written new tests, as applicable (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
I have added a changeset, if applicable

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature

What is the new behavior (if this is a feature change)?

Transforms now get access to a logger object through the context, which allows for logging at different levels (info, warn, error).

Removes the existing silent functionality.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this
PR?)
The way in which the warning is logged has changed. There might be a tiny chance that someone relies on parsing the plugin output to detect this, but this seems unlikely.
Other information:
Output from vite before this PR:

Output from vite after this PR:

changeset-bot · 2023-01-16T19:01:41Z

🦋 Changeset detected

Latest commit: 473f3a8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
imagetools-core	Patch
rollup-plugin-imagetools	Patch
vite-imagetools	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2023-01-16T21:06:20Z

Codecov Report

Merging #486 (473f3a8) into main (6291c71) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #486      +/-   ##
==========================================
+ Coverage   96.75%   96.94%   +0.18%     
==========================================
  Files          31       32       +1     
  Lines        1109     1145      +36     
  Branches      222      235      +13     
==========================================
+ Hits         1073     1110      +37     
+ Misses         36       35       -1

Flag	Coverage Δ
imagetools-core	`98.72% <100.00%> (+0.03%)`	⬆️
rollup-plugin-imagetools	`99.25% <100.00%> (+0.02%)`	⬆️
vite-imagetools	`84.66% <100.00%> (+1.33%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/core/src/lib/generate-transforms.ts	`100.00% <100.00%> (ø)`
packages/core/src/lib/logger.ts	`100.00% <100.00%> (ø)`
packages/core/src/transforms/format.ts	`100.00% <100.00%> (ø)`
packages/core/src/transforms/resize.ts	`97.41% <100.00%> (ø)`
packages/core/src/types.ts	`100.00% <100.00%> (ø)`
packages/rollup/src/index.ts	`99.25% <100.00%> (+0.02%)`	⬆️
packages/vite/src/index.ts	`84.66% <100.00%> (+1.33%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

tlaundal · 2023-01-16T21:39:22Z

packages/core/src/transforms/__tests__/resize.spec.ts

-    dirCtx = { useParam: vi.fn, warn: vi.fn }
-  })
+let dirCtx: TransformFactoryContext
+beforeAll(() => {


I had to extract this to the top level, because I got interference between the different describe blocks. Not sure what was causing this, but perhaps parallel test running or something similar?

This was the simplest way I found of adressing the issue.

packages/core/src/transforms/__tests__/resize.spec.ts

packages/rollup/src/__tests__/main.test.ts

packages/rollup/src/index.ts

packages/core/src/transforms/resize.ts

benmccann · 2023-01-17T05:05:54Z

I noticed prettier was reformatting some files in your PR. The whole repo should have been formatted, but the enforcement for it accidentally got turned off. I re-enabled it and formatted the repo, so you'll have to rebase this PR now

benmccann

I'm not entirely sure we need a new option as much as just lowering the level of the newly added log message. If we were to add a new option maybe it'd make sense to align it with Vite? https://vitejs.dev/config/shared-options.html#loglevel

benmccann · 2023-01-18T00:36:11Z

packages/core/src/transforms/resize.ts

@@ -91,8 +91,8 @@ export const resize: TransformFactory<ResizeOptions> = (config) => {
      finalWidth = originalWidth
      finalAspect = originalAspect

-      console.warn(
-        '[vite-imagetools] withoutEnlargement or withoutReduction enabled. Image width, height and aspect ratio reverted to original values'
+      context.warn(


This warning doesn't seem very useful. E.g. it doesn't tell you what image it encountered

I also wonder if this should even be a warning? This doesn't seem like an error condition, but just regular functioning. Maybe it should be something more like debug or info. I'm not that familiar with the feature though. @sawyerclick can you provide some more details?

@JonasKruckenberg recommended the warning as a part of his review in the original pr #461

Now that I'm looking at it again, I don't believe the warning is needed. It's redundant as it's sole purpose is to indicate that the flags are working.

Ah, I see: #461 (comment)

@JonasKruckenberg it seems this warning has been a bit noisy. Do you mind if we either remove it or demote it to an info or debug? I think if users need to figure out whether it was triggered they can always compare the dimensions on the filesystem to the rendered dimensions

My concern was that the whole idea of without* is to not do anything when the conditions are met. To the uninitiated (or tired) this might seem like the whole plugin not doing it's job though. Also in all cases I can imagine without* is a safeguard that trips when you made an error while specifying your image dimensions. That's why I originally recommended the warning, but I guess info makes sense too (since it technically works as intended)

Edit: And yes I agree the message itself also isn't that helpful yet

JonasKruckenberg · 2023-01-18T06:23:22Z

I'm not entirely sure we need a new option as much as just lowering the level of the newly added log message. If we were to add a new option maybe it'd make sense to align it with Vite? https://vitejs.dev/config/shared-options.html#loglevel

I would like to add more logs to this plugin to make issues easier to debug, but the specifics really don't matter here. Respecting the vite config value (when we're in the vite plugin) makes total sense to me, but we need a way to pass this on to the core crate. Maybe we can use an existing logger library

tlaundal · 2023-01-18T09:58:33Z

I won't get into the discussion of whether to log when without* takes effect, as long as it can be disabled.

Looking into the logging capabilities of vite and rollup, I have found that:

Rollup has warn and error functions, but nothing else it seems. The only difference is that error will abort bundling
Vite, of course, inherits these functions, but also introduces the config.logger property to access a Logger object, which supports info. Btw, in Vite, the plugin context .error aborts bundling same as in rollup, but the logger.error function does not

Based on this, I think it makes sense that the imagetools rollup and vite plugins provides an object with info, warn and error functions to the transformer context. In this case, vite can control the logging verbosity, but I don't think rollup provides such controls.

JonasKruckenberg · 2023-01-18T10:41:47Z

Based on this, I think it makes sense that the imagetools rollup and vite plugins provides an object with info, warn and error functions to the transformer context. In this case, vite can control the logging verbosity, but I don't think rollup provides such controls.

I think this makes sense, when used in a Vite context we can just proxy the log functions provided by Vite and in a the rollup case we just use the functions we're provided and maybe make the rest no-ops

tlaundal · 2023-01-18T11:18:52Z

in the rollup case we just use the functions we're provided and maybe make the rest no-ops

Yes, no-op or console.log are both valid choices I think.
One thing we should clarify is whether our error should abort bundling, like the plugin context error function, or just log the message, like what the vite logger does. (Vite also aborts in the case of the plugin context error function).

I can implement this tomorrow if there are no concerns from Ben or others.

Edit: Since Rollup doesn't provide logging levels, I propose both info and warn are forwarded to Rollup's warn. It can be silenced with a cli flag or with an onwarn handler.

docs/README.md

.changeset/bright-bears-bake.md

JonasKruckenberg

Looks good! Thanks for taking the time to fix this 👍🏻 One last thing though: I would argue this should be a patch fix too, since it doesn't introduce any new user-facing features, and instead fixes some weird behaviour

tlaundal · 2023-01-20T13:23:19Z

Looks good! Thanks for taking the time to fix this 👍🏻 One last thing though: I would argue this should be a patch fix too, since it doesn't introduce any new user-facing features, and instead fixes some weird behaviour

Haha, I originally had it as a major (because I completely removed the config option at first, instead of deprecating), so now we've been through the whole spectrum. I'll make the change to patch.

And thank you for creating this great library in the first place 🙌

Deprecates the silent option and delegates that functionality to the bundlers. Vite has log level controls, and Rollup let's the users manage the logging with the `onwarn` hook.

tlaundal commented Jan 16, 2023

View reviewed changes

tlaundal marked this pull request as ready for review January 16, 2023 21:43

benmccann reviewed Jan 16, 2023

View reviewed changes

packages/core/src/transforms/resize.ts Show resolved Hide resolved

tlaundal force-pushed the silence-withoutEnlargement-and-withoutReduction branch from dead22e to 94aa59f Compare January 17, 2023 07:08

tlaundal mentioned this pull request Jan 17, 2023

Image URLs break with HMR and the silent option doesn't help #437

Open

benmccann reviewed Jan 18, 2023

View reviewed changes

benmccann reviewed Jan 19, 2023

View reviewed changes

docs/README.md Outdated Show resolved Hide resolved

benmccann reviewed Jan 19, 2023

View reviewed changes

.changeset/bright-bears-bake.md Outdated Show resolved Hide resolved

benmccann approved these changes Jan 19, 2023

View reviewed changes

tlaundal force-pushed the silence-withoutEnlargement-and-withoutReduction branch from ef40720 to 29f3629 Compare January 20, 2023 09:08

tlaundal changed the title ~~feat: implement silent functionality~~ feat: add logging facilities Jan 20, 2023

JonasKruckenberg approved these changes Jan 20, 2023

View reviewed changes

tlaundal added 2 commits January 20, 2023 13:26

fix: log messages through Vite and Rollup

6842c73

Deprecates the silent option and delegates that functionality to the bundlers. Vite has log level controls, and Rollup let's the users manage the logging with the `onwarn` hook.

chore: regenerated docs

473f3a8

tlaundal force-pushed the silence-withoutEnlargement-and-withoutReduction branch from 29f3629 to 473f3a8 Compare January 20, 2023 13:30

JonasKruckenberg merged commit 93bc23a into JonasKruckenberg:main Jan 20, 2023

tlaundal deleted the silence-withoutEnlargement-and-withoutReduction branch January 22, 2023 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add logging facilities #486

feat: add logging facilities #486

tlaundal commented Jan 16, 2023 •

edited

Loading

changeset-bot bot commented Jan 16, 2023 •

edited

Loading

codecov bot commented Jan 16, 2023 •

edited

Loading

tlaundal Jan 16, 2023

benmccann commented Jan 17, 2023

benmccann left a comment

benmccann Jan 18, 2023

sawyerclick Jan 18, 2023

benmccann Jan 18, 2023

JonasKruckenberg Jan 18, 2023 •

edited

Loading

JonasKruckenberg commented Jan 18, 2023

tlaundal commented Jan 18, 2023

JonasKruckenberg commented Jan 18, 2023

tlaundal commented Jan 18, 2023 •

edited

Loading

JonasKruckenberg left a comment

tlaundal commented Jan 20, 2023

feat: add logging facilities #486

feat: add logging facilities #486

Conversation

tlaundal commented Jan 16, 2023 • edited Loading

changeset-bot bot commented Jan 16, 2023 • edited Loading

🦋 Changeset detected

codecov bot commented Jan 16, 2023 • edited Loading

Codecov Report

tlaundal Jan 16, 2023

Choose a reason for hiding this comment

benmccann commented Jan 17, 2023

benmccann left a comment

Choose a reason for hiding this comment

benmccann Jan 18, 2023

Choose a reason for hiding this comment

sawyerclick Jan 18, 2023

Choose a reason for hiding this comment

benmccann Jan 18, 2023

Choose a reason for hiding this comment

JonasKruckenberg Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

JonasKruckenberg commented Jan 18, 2023

tlaundal commented Jan 18, 2023

JonasKruckenberg commented Jan 18, 2023

tlaundal commented Jan 18, 2023 • edited Loading

JonasKruckenberg left a comment

Choose a reason for hiding this comment

tlaundal commented Jan 20, 2023

tlaundal commented Jan 16, 2023 •

edited

Loading

changeset-bot bot commented Jan 16, 2023 •

edited

Loading

codecov bot commented Jan 16, 2023 •

edited

Loading

JonasKruckenberg Jan 18, 2023 •

edited

Loading

tlaundal commented Jan 18, 2023 •

edited

Loading