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

feat: Add a logger to embed init options #732

Open
wants to merge 6 commits into
base: next
Choose a base branch
from

Conversation

challet
Copy link

@challet challet commented Aug 4, 2021

Add an option to the embed function.
It allows to pass a custom logger to the view.

@challet challet changed the title Add a logger to embed init options feat: Add a logger to embed init options Aug 4, 2021
src/embed.ts Outdated
@@ -10,6 +10,8 @@ import {
isString,
Loader,
LoaderOptions,
logger as VgLogger,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
logger as VgLogger,
logger as vegaLogger,

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

We should also pass the logger to Vega-Lite.

Please add a test as well.

README.md Outdated Show resolved Hide resolved
challet and others added 2 commits August 5, 2021 11:02
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
@challet challet marked this pull request as draft August 5, 2021 09:07
@challet
Copy link
Author

challet commented Aug 5, 2021

We should also pass the logger to Vega-Lite.

Isn't it already done since the part that adds the logger works with both Vega and Vega-Lite specs ? The mode having been guessed a few lines before.

@challet challet marked this pull request as ready for review August 6, 2021 09:14
@hydrosquall hydrosquall changed the base branch from master to next September 16, 2021 23:55
@domoritz
Copy link
Member

I don't think it is. We call compile in the preprocessor in https://github.com/vega/vega-embed/pull/732/files#diff-b0761852e43b4566f7d72f8bc18bc1a7fd5db43c5ad1d9dfcc4529afe86d0174R108. But we are not passing the logger in (docs at https://vega.github.io/vega-lite/usage/compile.html#javascript). Could you update the code to make it work? I think the best would probably be to remove the preprocessor variable and just conditionally call vl.compile on the spec.

@domoritz
Copy link
Member

@challet can you finish up the pull request?

@@ -304,7 +304,7 @@ async function _embed(

const mode = guessMode(spec, opts.mode);

let vgSpec: VgSpec = PREPROCESSOR[mode](spec, config);
let vgSpec: VgSpec = PREPROCESSOR[mode](spec, { ...config, logger });
Copy link
Author

Choose a reason for hiding this comment

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

Can be added directly to the passed config, since the whole arg will be discarded when the mode is not 'vega-lite'

@challet
Copy link
Author

challet commented Jan 31, 2022

@domoritz sorry it went out of my scope. I haven't added a test for the last change though and I will have a closer look at it.

@domoritz
Copy link
Member

domoritz commented Feb 1, 2022

Thank you. Can you make sure the test pass?

@domoritz
Copy link
Member

domoritz commented Feb 9, 2022

Ping @challet

@challet
Copy link
Author

challet commented Feb 9, 2022

Sorry, I will have a look in the next days.

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.

2 participants