-
Notifications
You must be signed in to change notification settings - Fork 113
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
Block helper rewrite #276
Block helper rewrite #276
Conversation
joining the args together without cutting out the options arg can add an '[object Object]' into the result.
this shouldn't be used as a block helper, the result may be non-portable
✔️ No visual differences introduced by this PR. View Playwright Report (note: open the "playwright-report" artifact) |
Nice, this looks great!! The more minimal syntax is a nice unexpected improvement. What is the expected experience for users after this PR? Will that templates upgrade file fix people's templates automatically in all cases, or is some manual action required? |
JP Mining Note uses automatic patching also can't be counted on for new users, because someone with an up-to-date install copying JPMN would not be subject to the patch process. manual work is required to be safe, which is why i included examples of the old and new syntax. |
@praschke Hmm. Could we maybe add a button to "Fix broken yomichan templates" in the settings which would run the template patcher for people? I think the template is idempotent so it should be a fairly safe thing to include. |
@djahandarie the settings upgrade process happens automatically, and a button would give the impression that all instances would be fixed automatically, when only the default templates are guaranteed to be fixed by the patch. i can't say that all templates in the wild would be fixed by such a button. |
Hmm, "Attempt to fix template" button then? 😄 Or if you think even that would be confusing / overcommitting, maybe an addition to the README would be most appropriate then? I think we need some sort of easily-visible guidance for people regarding how to fix their templates 🤔 |
as well as the readme, i think the welcome page could be used to alert people after an update in case they've modified their templates from the default. i can look into that today or tomorrow. |
7c322dc
to
d93a177
Compare
d93a177
to
e5edf90
Compare
e5edf90
to
7577075
Compare
there is now a migration guide in the readme. for new users of yomitan or users migrating from yomichan or yomibaba, the welcome page notes the situation and links to the guide in the readme. if someone has custom templates and yomitan updates, then the welcome page will open once and a cautionary notification will be highlighted in red with a similar note and link. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I think this should cover most cases for users! Left one minor comment, otherwise good to approve.
Closes #246, closes #277.
in handlebars, helpers can be passed information through positional or labeled arguments as well as two templated blocks, the truthy
options.fn()
and the falsyoptions.else()
. these block arguments can be evaluated under whatever context as many times as the helper wants, and they nominally return strings. block helpers (as opposed to regular helpers) are only useful for control flow or composition of effects.for some reason, handlebars does not always return strings as documented and specced, but objects. when evaluated,
{{{.}}}
gives the context object,{{{definition}}}
gives the definition object from the context. in contrast, kibana's no-eval handlebars (written in typescript) returns the stringified counterparts,'[object Object]'
. when yomichan's helpers attempt to treat the result of blocks as a non-string, things break.the helpers affected by this behavior are
dumpObject
furigana
furiganaPlain
formatGlossary
uses of these helpers must be rewritten into non-block helpers, as normal arguments are capable of passing javascript objects from the context to the helper.
in general, block helpers have been overused in yomichan, meaning the default templates and templates written by the community have been more verbose than necessary. this PR also changes the default templates and helper documentation to reflect better practices.
another bug was found due to
regexReplace
andregexMatch
not excluding the final options object from the arguments iterated over, resulting in[object Object]
sometimes being placed in the output.the deprecated
kanjiLinks
andsanitizeCssClass
have been removed.the
typeof
helper now has non-portable behavior with regard to block arguments. the no-eval path should probably be taken on both chrome and firefox instead of usingcompileFnName
to dynamically dispatch.