feat: run declared JavaScript without compiling it in the browser - #25749
Conversation
Focusable.focus() and blur() schedule their JavaScript through the new Element.executeJs(JsCommand), so the pending invocation carries a typed FocusCommand or BlurCommand. A driver of the client side that cannot run JavaScript recognizes the invocation by the type of its command instead of by matching the text of the generated expression, which is the framework's script wrapped by executeJs. The expression and the parameters sent to a browser are unchanged. Part of #25734
Replaces the per-operation command records with the invoker shape from #10759: the JavaScript is a constant on a FocusJs interface, obtained through the new Element.getJsInvoker(Class) and called as a Java method. The scheduled invocation carries a JsInvokerCall, so a driver of the client side can dispatch on the interface and the method, or run the call on its own implementation of the same interface. Kept as an alternative to the parent branch for comparison. The expression and the parameters sent to a browser are unchanged. Part of #25734
Type of changePrototype, draft on purpose, and an alternative to its own base rather How to testThe three generated scripts (focus, focus with options, blur) are byte API changes relative to #25747// Added
public @interface JsExpression { String value(); }
public record JsInvokerCall(Class<?> invokerType, String methodName, List<Object> arguments) implements JsCommand
public Object JsInvokerCall.invokeOn(Object implementation)
public interface FocusJs extends Serializable // focus(), focus(ObjectNode), blur()
public <T> T Element.getJsInvoker(Class<T> invokerType)
// Removed
public record FocusCommand(List<FocusOption> options)
public record BlurCommand()
The two shapes, side by side
Two things that only showed up once both existed:
In exchange, What is still missing for #10759This is only the server half. CSP compatibility needs the client On the trigger/action prototypeIt already answers the question for its own family, and it answers it Both prototypes here apply that to one-shot Open questions
@Artur- @mcollovati @Legioth — the two shapes are now the same feature |
An interface annotated with @JsInvoker declares the JavaScript the server can invoke as @JsExpression constants, and Element.getJsInvoker(Class) calls it as a Java method. The build collects the declarations into the bundle, the response names the interface and the method instead of carrying a script, and the client runs the function from the bundle. Nothing is compiled in the browser, so the call needs no unsafe-eval, and the JavaScript an application can be made to run is known when it is built. Focusable.focus() and blur() are the first users. A bundle that was built without the declared JavaScript is rebuilt, and the dev mode class finder knows the annotation, so the functions are there when a call looks them up. Part of #10759 Part of #25734
The error cases of getJsInvoker and JsInvokerCall had no tests, which is what the coverage gate on the pull request flagged. The return type is now also checked before the call is scheduled, so a method the invoker can not answer does not reach the browser, and what an implementation throws in invokeOn reaches the caller instead of being wrapped.
totally-not-ai
Bot
changed the title
Type of changePrototype, still a draft, but now the whole chain rather than the server How to test
What the chain looks like nowThe response for "execute":[[{"@v-node":9},{"invoker":"com.vaadin.flow.component.FocusJs","method":"focus/0","arguments":0}]]No JavaScript is sent. The client takes the element ( Measured in the browser by counting every call of Two things the end-to-end run turned up
How far this gets CSPNot all the way, and the measurement says exactly how far. Served with That is the bootstrap, not this change: dependency loading evaluates For a driver that can not run JavaScriptThe queued invocation carries the call, so the browserless case is a Open questions
|
The bundle check compared a hash the bundle never carried, since the Vite stats only hash a known set of files and the generated invoker file was not one of them. Every application therefore looked out of date and rebuilt its bundle, which an application that runs on a precompiled bundle can not do at all — the no-plugin tests caught it. The stats now hash the generated file the way they hash the commercial banner, so a bundle carries what its invokers declared, and a bundle built before invoker interfaces existed is left alone instead of forcing a rebuild that would not help.
📦 Snapshot published
Built without running tests. Look at the checks on this pull request before relying on it. Every new commit on this branch republishes it while the How to use it<repository>
<id>vaadin-snapshots</id>
<url>https://maven.vaadin.com/vaadin-prereleases</url>
<snapshots><enabled>true</enabled></snapshots>
</repository>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>flow-bom</artifactId>
<version>25.4.js-invoker-seam-for-focusable-SNAPSHOT</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>Import |
The client located the element to apply the function to by index, trusting the argument count that came with the invocation. An invocation built against another signature would have bound an argument as `this` and run the call with everything shifted. The count is now compared with what the target declares and the mismatch is reported instead of running. What an invoker interface declares is also a frontend change now: the JavaScript is generated into the bundle, so the dev loop escalates to a restart for it, the same way it does for a JsModule. The declared expression is part of the comparison, since editing one keeps the method it belongs to and would otherwise go unnoticed.
A call the client refused to run because its parameters did not add up left the two return channels untouched, so a call that was subscribed to never completed on the server and the application's handler never ran, with a line in the browser console as the only trace. The channels are appended after everything else, or not at all, so the last parameter is the error channel even when the count in front of it is wrong, and the message now goes through it.
A class redefined straight from an IDE never reaches the dev loop, so the escalation to a restart that a changed invoker interface needs did not happen there and the browser kept running the JavaScript the bundle was built with, silently. A hotswapper now compares what a redefined @JsInvoker interface declares with the generated file the bundle was built from, which is what the browser can actually run, and reports the ones it does not carry. There is nothing to apply in the browser instead: only a frontend build produces the new function.
Looking for the method id and the expression anywhere in the generated file passed things it should have reported: an interface renamed or moved keeps its methods and JavaScript in the bundle under the name of before, and an expression shortened to a prefix of what the bundle carries still matched. The comparison now uses the rendering the build wrote, so the interface name, the methods, their argument counts and the JavaScript all have to match. The tests drive the hotswap event instead of the comparison, which also covers resolving the generated file and ignoring a class that declares nothing.
With the frontend dev server running there is no reason to ask for a restart: the file the functions are generated into is now written again from what the interfaces declare, and the file accepts its own update, so the dev server replaces that module in every browser that has it and a call made afterwards runs the new JavaScript. Nothing is compiled from a string in the browser, since what the dev server serves is the file it just read, and the page is not reloaded. Without the dev server a bundle is what the browser runs, and only a build produces a new one, so the change is still reported there.
Regenerating it went through a class finder from the context lookup, which nothing puts there: the lookup resolved to nothing, so the write never happened and every change was reported instead of applied. The file is now rendered from the interfaces it already holds - which are what the browser has, and none of them changed - plus the ones that just changed, so nothing has to scan, and an interface that was only now annotated gets in as well. Writing is also no longer taken for success on its own: what the file holds afterwards is compared with the declarations again, and whatever it does not cover is reported, so a change nobody can apply is never silently swallowed.
The types are not about the DOM: the same declarations are what Page.executeJs would be invoked through, so they now live in com.vaadin.flow.js. The interface behind Focusable is a nested interface of it, rather than a file of its own, so the declarations sit with the code that calls them. Its two focus methods are one: passing no options passes null, which a browser reads as the empty set of options it would use anyway, so the choice between them is gone from both the interface and the caller. An argument of a call may therefore be null.
Their javadoc links the entry point they are called through, which resolved while they sat in the same package as it. The import they need for that is back, so the javadoc build has something to resolve again. An argument being allowed to be null is also pinned where it is decided: one case for a call keeping it and handing it to the implementation, the way focusing without options does.
|
Should this same PR handle Page level executions also? |
|
I think this PR is already big enough. We can do |
Writing the file again while the application runs put the names of the functions it adds in front of what closes the file, which throws when the file was written before names were rendered at all - a build made by an earlier version of this, or a production build read in development. The registry is added with them now, and both places that open a file build the opening the same way. The cases for writing the file again ran as a production build, which is the one mode that path never runs in, so they run as a development build too: what is added carries its names, the registry that holds them comes first, and a file that already carries everything is left alone. On the client, the two registries are read through one accessor rather than through the same cast written twice.
| Collection<Class<?>> definitions) { | ||
| String generated = readGeneratedFile(options); | ||
| String content = withMissingEntries(generated, definitions, | ||
| !options.isProductionMode()); |
There was a problem hiding this comment.
There is no point in chrcking production mode in a dev only class
There was a problem hiding this comment.
Right - findMissingFromGeneratedFile and updateJsDefinitions are only called while the application runs, so they render with the names unconditionally now and the mode is asked about only where a build decides what it writes. The cases for those two ran as a production build, which is the one mode they never run in, so they run as a development build now.
| * into, and the one a name is assigned into when names are rendered. Both | ||
| * assign only what is not there, so a file can carry them more than once. | ||
| */ | ||
| private static List<String> header(boolean withNames) { |
There was a problem hiding this comment.
”header” what - methods should have a verb
There was a problem hiding this comment.
Renamed to renderHeader, next to the renderFileContent and renderDefinitionLines it belongs with.
|
|
||
| @Override | ||
| protected boolean shouldGenerate() { | ||
| return options.getClassFinder() != null; |
There was a problem hiding this comment.
It would not be - a build that scans for anything has a class finder, and the file is written whenever the frontend is generated. The check is gone.
| if (command.length > 0 && command[0] === 'window.location.reload();') { | ||
| const runs = whatInvocationRuns( | ||
| command, | ||
| (valueMap.constants ?? {}) as Record<string, unknown>, |
There was a problem hiding this comment.
Should not constants first be put in the pool?
There was a problem hiding this comment.
It no longer does - see the reply on the next line.
There was a problem hiding this comment.
They are now: the constants of a message go into the pool as it arrives, before anything decides what to do with the message, and what an invocation runs is read from the pool alone.
That needed one thing: a message that is queued here is read again when it is handled, so the same constants are imported twice. A key is a hash of the value it names, so the second import is the same value, and ConstantPool.importFromJson takes a key it already holds instead of asserting on it.
| * @param constantPool - what earlier messages put in the pool | ||
| * @returns what to run, or `null` when nothing is named | ||
| */ | ||
| export function whatInvocationRuns( |
There was a problem hiding this comment.
Renamed to resolveWhatRuns, and it takes the constant pool alone now that the constants are in it before anything reads them.
| * development bundle registers next to the function itself, and the identifier | ||
| * of the function when it does not, as in production. | ||
| */ | ||
| function nameOf(functionId: string): string { |
There was a problem hiding this comment.
Verb in method name here and many other places
There was a problem hiding this comment.
Renamed the ones this change added: getDeclaredJavaScript for the registries the bundle populates and getNameOf for what a message calls a function by. The others in the file - findDeclaredFunction, reportThroughChannel, invokeFromBundle - already read as verbs.
| * @return the function identifier, not <code>null</code> | ||
| */ | ||
| public static String functionId(String expression, int argumentCount) { | ||
| return StringUtil.getHash(argumentCount + ":" + expression, |
There was a problem hiding this comment.
Is the argument count relevant here?
There was a problem hiding this comment.
It is, because the parameter list of the generated function is made of it: the same expression at one argument and at two is async function ($0) and async function ($0, $1), two different functions. Hashing the expression alone would give them one identifier, and whichever the build wrote last would be what both calls run.
It is also what the client reads the invocation with - the function takes the arguments, so its arity says where the element is - so two arities have to stay two functions.
…runs when The constants of a message go into the pool as it arrives, before anything decides what to do with the message, since what an invocation of it runs is read from there - which is what tells a forced reload apart from any other invocation while a resynchronization is ongoing. A message that is queued is read again when it is handled, and a key is a hash of its value, so the pool takes the same key again. Writing the generated file while the application runs is only ever a development build, so it no longer asks which mode it is in, and the cases for it run in that mode too. Whether a build writes the names stays where the build decides it. Names with verbs, in the places this change put them: renderHeader, resolveWhatRuns, getDeclaredJavaScript and getNameOf. And the generated file is written whenever the frontend is generated, rather than asking whether there is a class finder, which a build always has.
The message they are imported from is read again when it was queued, which is not the same as when it is processed - the processing side reads the pool rather than filling it. Generating the file needs something to scan the definitions with, and a caller that writes the file again knows them already and passes them in, so going through the generating side without a class finder says which of the two it is rather than failing on a null.
The pool said again that a key arrives once, and the message handler keeps that true: a message that is queued and read again is remembered, so its constants go in as it arrives and not a second time. The case for importing the same key twice goes with it, and one for a message that is read twice takes its place. The processing side no longer says where the constants went in, since nothing there does anything with them.
The pool threw on a key it already had, and a message can reach the client more than once - the server re-sends one, and the client ignores it as already seen - so the constants that message carries would have thrown out of the reading that happens before that. A key is a hash of the value it names, so a key that is already there is taken as the value that is already there, and what is refused is a key that names something else. The handler no longer remembers which messages it has read, since nothing depends on reading one only once. The case that went with the bookkeeping is replaced by the two that say what the behaviour is: a message the server re-sends is read again without anything being thrown, and the pool refuses a key that names a second value.
|
@claude review |
|
Claude finished @Artur-'s task in 5m 20s —— View job Code reviewI read the full change end-to-end (server
Worth a decision — session constant pool now retains every distinct expressionWith this change, For the common case where an expression is reused this is a clear win (sent once, named many times). But code that builds a unique expression string per call — e.g. Minor
Things I checked and found correct
Nice work — the |
Let's add "f": "id" instead of "id" or something similar so we don't have a collision risk |
Let's fix this for clarity |
…ke one
The constant an invocation of declared JavaScript names was the
identifier of a function, a string, and so is the constant an invocation
of an expression names. Telling them apart went by the shape of the
string, which an expression of exactly that shape would have fooled.
The constant is now `{"f": "<identifier>"}`, and what tells the two
apart is that one is an object and the other is a string. It costs the
two characters of the key once per function, since a constant is sent
once.
…like The point of naming a function in an object is that nothing else can be taken for one, so a case runs an invocation whose constant is a string of exactly the shape an identifier has and asserts that it is run as an expression. A check by shape would pass the suite without it. The key of that object is written once on each side now: the client mirror of the JSON constants carries it, and the type of the constant is built from it. The assertion that the constant is an object naming the function was already made by the comparison above it, which is against exactly that object.
A method that declares no JavaScript and could not be answered with what it returns went into both lists, and only the first of them is ever said. It goes into the one that names what to do about it, so what the lists hold is what they say they hold.
… message The commit before this put a method that declares nothing into that list alone. Nothing a caller sees changed: the first list with anything in it is the one that is reported, and that has always been the one about declaring. A comment says so where it could be read the other way.
|
Add a section to the Calling JavaScript page explaining @JsDefinition and @JsExpression, and Element.executeJs(Class), which runs JavaScript that the build collects into the bundle instead of compiling an expression in the browser, so the call works under a content security policy without unsafe-eval. Cross-reference it from the existing executeJs(String, Object...) section, which had no mention of that limitation. Documents vaadin/flow#25749 (`39953e133526e0b268f7c24fd27bbc760bec34ea`).
|
Pull request created: #6113
|
|
Documentation Bot: Draft documentation pull request for this change: vaadin/docs#6113 Files updated:
It was written from the state of this pull request as you see it now. Please review it and mark it ready for review.
|
4 participants
Summary
A Java interface can now declare the JavaScript it runs, and the build collects that JavaScript into the frontend bundle. The server then tells the browser only which generated function to run, so server-initiated JavaScript no longer has to be compiled from a string and works under a Content Security Policy without
unsafe-eval.What changed
Behavior change: what
executeJsputs on the wire (affects anyone reading the raw UIDL). Every scheduled invocation in theexecutearray now ends with a constant-pool key instead of the script text. The script itself is sent once per session underconstants. Application code is unaffected, but custom clients or test tooling that read the script straight out ofexecutemust resolve the constant first. The MPR push-state fix-up inUidlRequestHandleris updated here.Behavior change: an existing frontend bundle is rebuilt once. The generated definitions file is hashed into the bundle stats, so a bundle built before this change counts as out of date and is rebuilt on the next build or start.
Behavior change:
Focusable.focus(...)andblur()go through the new path. The visible effect is the same, but a driver that inspects pending invocations now sees aJsCallinstead of an expression, andfocus()without options passesnullto the browser'sfocus()rather than calling it with no arguments.Everything else is additive:
@JsDefinition/@JsExpressionannotations andElement.executeJs(Class), which hands out an implementation of the interface. Calling a method schedules the JavaScript the method declares, with the call arguments as$0,$1, … and the element asthis— the same contract asexecuteJs(String, Object...). The interface is checked when the implementation is handed out.TaskGenerateJsDefinitionswritesgenerated/vaadin-js-definitions.jswith one function per declared expression, keyed by a hash of the JavaScript. The bootstrap and web-component bootstrap import it, and Vite hashes it into the stats. A production bundle carries the functions only — the Java names stay on the server.JsCall, so a browserless driver can recognize it or run it on its own implementation of the same interface.ExecuteJavaScriptProcessorresolves what to run from the constant pool: an object constant means a bundled function applied to the element, a string constant is an expression as before. Missing functions and mismatched argument counts are reported through the error channel so a pending result is not left hanging.JsDefinitionHotswapperrewrites the generated file when a definition class changes (and warns when it cannot), andDevLoopRedefinertreats the declared JavaScript as a frontend dependency.Use case
An app has to run under a strict CSP that forbids
unsafe-eval, and a "Copy order id" button should put a value on the clipboard and report whether it worked. With a declared definition, the JavaScript lives in the bundle and nothing is compiled in the browser.API Changes
API Changes: feat/js-invoker-seam-for-focusable vs origin/main
12 classes affected, 23 members added, 0 removed, 0 changed.
com.vaadin.flow.js.JsDefinition
com.vaadin.flow.js.JsExpression
com.vaadin.flow.js.JsCall
com.vaadin.flow.js.JsDefinitionProxy
com.vaadin.flow.dom.Element
com.vaadin.flow.component.Focusable.FocusJs
com.vaadin.flow.component.internal.UIInternals.JavaScriptInvocation
com.vaadin.flow.internal.ReflectTools
com.vaadin.flow.internal.FrontendUtils
com.vaadin.flow.shared.JsonConstants
com.vaadin.flow.server.frontend.TaskGenerateJsDefinitions
com.vaadin.base.devserver.hotswap.impl.JsDefinitionHotswapper
Test summary
Declaring and calling JavaScript from Java:
focus/blurdispatched asFocusJscalls, plainexecuteJsleft without a callUIDL encoding:
{"f": <function id>}, carrying neither the expression nor the declaring classGenerated definitions file:
Bundle validation and dev mode:
Client: