Load user-customisations lazily

[tecosaur]

This is primarily motivated by reducing the amount of work that trimming is unable to remove.

It will require applications (by which I mean the actual thing that uses StyledStrings, for example the REPL) to call load_customisations! itself. If there's a good spot to add that so it automatically gets done "at the right time, when appropriate to do so" that would be even better, but I'm not sure there's an obvious spot that can be done without the potential for headaches (e.g. what if it comes up within a withfaces call).

For now at least, perhaps this is a reasonable compromise.

[JeffBezanson]

I'm not sure it's worth doing this way; all users needing to call this manually seems like a much worse API and of course carries the risk that they will call it from their init method 😄

As an alternative, we're ok as long as it's easy to avoid loading this package, i.e. Base and most stdlibs don't depend on it.

[tecosaur]

I'm not sure it's worth doing this way; all users needing to call this manually seems like a much worse API and of course carries the risk that they will call it from their init method 😄

The reason why I'm proposing this is because I think (perhaps I'm mistaken) the vast majority of users won't need to call this method manually. Most interactive/rich output comes up in the REPL, and if just the REPL calls load_customisations!() then nothing else will need to.

User scripts and certain statically-compiled files are the two cases that come to mind where a user might want to call it manually, but maybe that's a worthwhile tradeoff?

[JeffBezanson]

OK, that's good news. I'm ok with this then.

[tecosaur]

Let's give this a shot then 🙂

[KristofferC]

Most interactive/rich output comes up in the REPL, and if just the REPL calls load_customisations!() then nothing else will need to.

Ok, but then when you all of a sudden run something outside the REPL things will no longer work? So you might have code that works as you expect because you have a transitive dependency on the REPL but when that dependency is dropped by some of your own dependencies all of a sudden you lose all customisation.

[tecosaur]

Yea, different visuals when running things in vs. out of the REPL is the potential downside (and load_customisations! is not explicitly called).

I wonder if maybe this could be a good spot to use Preferences.jl so that this init step can be enabled by default, by disabled when trying to static-compile minimal executables?

[KristofferC]

I think this is not a good way to do this and should be reverted. If something like this is to be done it should be done actually lazily by having the library itself initialize the customizations the first time it uses something that needs it. Not requiring an explicit call to it.

[tecosaur]

I think this is not a good way to do this and should be reverted.

I'm open to reverting this. With the interest in removing/reducing __init__ tasks though I'd like to make it so that StyledStrings isn't contributing to the problem, and making it so that TOML parsing is potentially trimmable (this was flagged on Slack).

I wonder if that Preferences idea I mentioned above might be a viable way of approaching this compromise from another (better) direction?

If something like this is to be done it should be done actually lazily by having the library itself initialize the customizations the first time it uses something that needs it. Not requiring an explicit call to it.

This is how I originally wanted to approach this, but it's not entirely straightforward which makes me slightly wary. For instance, we could just add it to the printing in io.jl, but then if printing occurs within a withfaces call then that risks the wrong styling being applied to both the active context as well as the top-level context values.

Loading on first printing/withfaces invocation might be viable though? I'll want to roll this idea around for a bit longer before giving it a shot though.

[topolarity]

I don't think the lazy init approach solves our original problem, which was minimize the "surface area" of StyledStrings, esp. w.r.t. to code size + dependencies.

I'm not sure the TOML parser is actually a very heavy dependency w.r.t. code size (it's self-contained and a pretty simple grammar/spec) - might be worth a measurement. But it does seem undesirable to have, e.g., all Julia-built CLI apps dynamically loading StyledStrings faces on startup / first print.

[topolarity]

TBH, I feel skeptical about the global Faces dictionary in the first place...

It seems like it creates a namespacing problem. If I called addface!(:emphasize => ...) in my library there's no guarantee that I won't pick up a conflicting :emphasize definition from another library. This makes it hard to make sure things will print "right" once composability is considered.

Even providing a global dictionary of Faces as a TOML seems challenging to me. If the "theming" included a standard set of Face names, so that the intended purpose of each Face was more-or-less standardized it would be more plausible to me that you could do a "palette swap" for accessibility, etc. without ruining printing for someone.

I feel like instead of working with a global dictionary of Faces, this feature should either (a) be removed, or (b) be reworked to work in terms of complete "palettes" instead of a global dictionary of Faces.

[tecosaur]

Cody and I have have had a chat about the concerns he raised in the above comments, and I think it's fair to say that we both see the single global faces dict as non-ideal, but not a terrible compromise. He's proposed an idea of "Palettes" that we're both interested in exploring and could come along in the future.

This is separate to the near-term future of the changes that Kristoffer has expressed dissatisfaction with. From here, our options are to:

1. Make no further changes
2. Revert this PR
3. Use a compile-time preference (can we do this reasonably without Preferences.jl, given it's not in the stdlib?), to set whether load_customisations! is called on init or not
4. Come up with a better balance of concerns/behaviour

The relevant considerations I'm aware of currently are:

• Face customisations are done via parsing a TOML file
• This was done unconditionally as an __init__ step
• For static compiling in particular, we want it to be possible to not do this
• load_customisations! achieves this, but runs the risk of non-REPL-run/static-compiled code behaving differently to how people expect

This makes me wonder if perhaps a good (4) would be something like this (it it possible?)

@static if !we_are_static_compiling()
    const __init__ = load_customisations!
end

[tecosaur]

I think #94 should resolve this decently well 🙂.