Pre-release quality pass - #908
Merged
Merged
Conversation
balat
force-pushed
the
quality-pass
branch
2 times, most recently
from
September 26, 2026 16:57
4a5bdaa to
f0a31b2
Compare
A mechanical edit had put open Lwt.Syntax on the first line of 22 files, above the licence header and the module description. Move it next to the other opens, after those comments. The order of the opens is unchanged.
- Client_core.delay, never called. - In ppx_eliom_server, the flush wrapper that filled global_known and computed a list of fresh injections, then returned the unfiltered list; global_known was never read. - Service_base.Unreachable_exn, never raised (the module has no interface, so it was exported, but nothing uses it). - An li element built and thrown away in Tools.menu. With xhr links, building it registered an unused onclick client value in the request data; the page no longer carries it. - Utils.todo and two empty doc comments in the tools. - Mod_sessiongroups.getsessgrp, the identity, never called. - The second of two identical Process.set_ignored_params calls in Client_main.
Remove the remnants of the old 204-based forms (204FORMS), superseded State and Mod_sessadmin functions, and one-line leftovers. Where a block also explained a decision, the explanation is kept.
Remove the old noscript-based CSS preloading, the set_tracing and URL-hash tracing hooks, the Marshal-based header encoding, and a section header with nothing under it.
Remove camlp4-era patterns, the disabled camlp4 dump in eliomc, and commented-out debug prints.
Also drop a commented-out record field in the Comet handler type.
- Name restore_history_dom in its comment (it said replace_page_in_history). - Put the explanation of persist_document_head above it instead of after it. - Drop a note about js_of_ocaml 1.1.2 and the commented-out code it referred to.
- State.create_volatile_table_during_session_, an undocumented alias of Mod_datasess.create_volatile_table_during_session used once. - Mod_cookies.compute_session_cookies_to_send, only called through its alias compute_cookies_to_send; keep the latter name. - module Opt = Option in Client, used twice.
The error promise of a client bus was derived from the Lwt.wait promise by a bind ending in assert false, wrapped in an identity Lwt.catch, only to give it the type of the stream elements while the resolver kept the type exn Lwt.u. The resolver is only used with Lwt.wakeup_exn, so give both the same type and use the pair directly.
- The Lwt.catch of Registration.typed_apply matched Eliom_Wrong_parameter only to fail with it, as with every other exception: use Lwt.fail as handler. The catch itself is needed and now says why: the parameter promises are already resolved, so the handler runs synchronously inside the bind, and the catch turns its exceptions into a rejected promise. - Parameter_base.reconstruct_params and State.Ext.lwt_check_scopes spelled out try Lwt.return (f x) with e -> Lwt.fail e; use Lwt.wrap and Lwt.wrap2. - raise (failwith ...) in Mkreg: failwith already raises.
get_secure has no optional argument, so its trailing unit parameter was useless and every call ended with ().
Replace matches that spell out Option.map, Option.value and Option.is_some, and the local option_map of Eliom_form. Option.value is only used where the default is a constant or a variable, since it is evaluated eagerly.
Replace List.concat (List.map ...), List.flatten (List.map ...), the flatmap helper of the ppx and the recursive map_include of the tools with List.concat_map, which applies the function in the same order.
- Re.execp instead of catching Not_found around Re.exec. - Sys.getenv_opt instead of catching Not_found around Sys.getenv. - Lwt_stream.flatten instead of Lwt_stream.map_list (fun x -> x). - Node_id_set.of_list instead of a fold of Node_id_set.add. - Fun.id instead of (fun x -> x).
Several log messages built their format as ("message" ^^ "@\n%s"). Write
the literal "message@\n%s" instead.
- string_of_result spelled out the four combinations of two booleans; use %b. - make_uri special-cased the empty path for String.concat, which already returns "". - A log message concatenated its text before formatting it with "%s". - Url.has_get_args caught Not_found from String.index; use String.contains.
An Events channel kept its React event alive in a field of type Obj.t option, set after building the channel through a match with an assert false branch. The value is a bool React.event, and the fold closure only needs a mutable cell to drop it on overflow: allocate that cell first, as a bool React.event option ref, and build the channel with it. No behaviour change.
The js_of_ocaml ppx drops the suffix after the last underscore, so ___eliom_appl_process_info_foo already denoted __eliom_appl_process_info. Use the trailing underscore, as for ___eliom_server_; the generated property name is unchanged.
Common.foundornot (Found of 'a | Notfound of 'b) was used as an option in Route.make_naservice (always Notfound ()) and as a result carrying the exception in Route_base.find_page_table. Use the standard types.
- Common: Vide and AVide constructors become Empty and AEmpty. - Eliom_uri.reconstruct_relative_url_path: aremonter, aaller become up, down. - Tools.menu: deplier becomes expand. - Utils: longueur, deb, beg, endd become len, start, stop. - Error_pages: deb becomes acc.
Mod_dom.ancessor becomes Mod_dom.ancestor (internal module), local childrens variables become children.
- Common.make_full_state_name2 site_dir_string secure ~scope becomes make_full_state_name_of_sitedata ~sitedata ~secure ~scope: every caller passed get_site_dir_string of a sitedata. - Mod_sessadmin.close_all_*_states2 become close_all_*_states_of_name (they take a full state name). - Mod_persess.close_persistent_state2 becomes close_persistent_state_of_cookie, and Mod_sessiongroups.Pers. close_persistent_session2 becomes close_persistent_session. - In State, fold_sub_states_aux_aux becomes prepare_sub_states_fold and the local f__ functions become find_or_create_cookie.
The global reference holding the reload function of the current page had the same name as the reload_function field in which it is stashed for the pages of the history.
The timeout setters took two adjacent positional booleans (whether to override a value set by the configuration file, and whether the new value comes from it), and callers wrote true true or override_configfile false. Label them.
Every caller passed the site directory string and the two subnet masks of the same sitedata as three positional arguments (two of them ints, easy to swap). Take ~sitedata and compute them inside.
rebuild_class_list took three positional string lists (current classes, classes to remove, classes to add), and one caller passed the same list twice to remove and re-add it. Label them ~current ~removed ~added, use List.mem, and pass ~removed:[] in that caller, which is equivalent.
Eliom was declared to build with OCaml 4.12, but two of its dependencies, ocsigenserver and wasm_of_ocaml-compiler, require 4.14, and CI only tests 4.14 and 5. Raising the bound lets the code use the Stdlib functions of 4.13 and 4.14.
Replace the hand-written prefix and suffix tests (String.sub with a length check, or a catch of Invalid_argument) by the Stdlib functions, available since OCaml 4.13. This also removes the string_prefix helper of Tools and makes remove_prefixed_param a List.filter.
Drop the parent-walking fallback of ancestor for browsers without Node.compareDocumentPosition (Internet Explorer 8 and older).
Drop the fallbacks for browsers without querySelectorAll or classList (Internet Explorer 8 and older), which walked the DOM and tested the class names by hand. The has_classes helper, which returned four booleans and was only used by these fallbacks, goes with them.
Drop the createEventObject fallback for Internet Explorer 8 and older.
copy_element worked around appendChild failing on script and style elements, and className not being updated, in Internet Explorer 8 and older. Append the copied children directly; the class attribute is copied with the other attributes.
Drop the styleSheet.cssText branch for Internet Explorer 8, where setting innerHTML on a style element failed.
Drop the code paths for browsers without history.pushState (Internet Explorer 9 and older): the URL fragment encoding of the current page (#!uri), its hashchange handler, and the fallbacks of Request_info that read the path from the current URL. Process.history_api stays, as it is exported.
It was only used by the code for browsers without the History API, and fell back to polling location.hash for browsers without the hashchange event.
Drop the attachEvent fallback of Internet Explorer 8 and older, and document in CHANGES the browser features that the client side now requires, and the removal of Mod_dom.onhashchange.
select_nodes returned a 5-tuple of node lists, and relink_page_but_client_values passed two of them on as a pair. Both now return a selected_nodes record. Mod_dom.select_nodes is exported, so the change is noted in CHANGES.
The state types stored in the History API were split between two blocks that disabled warning 39. They are now declared in one block.
Page_status was defined in the middle of the URL functions, far from Page_status_t and the page type it builds on.
It sat between the unwrappers and the History state types, while its only use is in init, for the handlers defined below it.
The unwrappers of client values, TyXML nodes and global data were registered far from the one of server functions, which has to come after call_ocaml_service. They are now registered together. Nothing is unwrapped while the module is being initialised, so registering them later does not change behaviour.
lock_request_handling, unlock_request_handling and wait_load_end join the other aliases kept for the interface. The only internal use of wait_load_end now calls Client_core directly.
Both were local to the block that installs the popstate handler, which made that block 150 lines long. The block now only installs the handler.
The code that binds links, forms, unique nodes, event handlers and client attributes after a page is received depends only on Client_core and Mod_dom. It moves, unchanged, to its own module with an interface exporting the four functions Client uses.
The page records, Page_status, the History module and the data saved for each state of the History API move, unchanged, to their own module. Client opens it, since pages are used throughout, and re-exports Page_status and History as before.
Building a request to a service, sending it, and leaving the application (exit_to, window_open) move, unchanged, to their own module. Client re-exports call_service, exit_to and window_open. The call of OCaml services stays in Client, as it initialises the request data.
get_tab_cookies finds the tab cookies in the request cache after an action, otherwise in the POST parameters or in the HTTP headers.
get_session_info reads the client process info, the expecting-process-page flag and the cookie substitutes through get_client_process_info, get_expecting_process_page and get_browser_cookies.
split_request_params separates the non-localised and ignored parameters of a request, or takes them from the request cache after an action.
get_service_params removes from the GET and POST parameters the ones that identify the non-attached service or the state of the request. It returns a service_params record instead of a 5-tuple with nested pairs.
Common.get_sp and the registration of a service with a session, session group or client process scope raised Failure when called outside a request, the latter with the unhelpful message "Missing sp while registering service". Site_information_not_available does not fit: the site information is available during the initialisation phase, it is the request that is missing. Both now raise the new exception Request_information_not_available, whose argument tells where the request was needed. For get_sp it keeps the Lwt call stack that the Failure message carried. The documentation of register and get_sp mentions the case, and an error during the initialisation of a site reports it with an explanation.
This was referenced Sep 27, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A pre-release quality pass on the whole Eliom code base. The commits are small and each does one thing. Every commit builds.
It follows #907 (latent bug fixes, merged). Unlike #907, it is meant to be behaviour-preserving, except for the last lots (Q6, Q7), which have CHANGES entries.
It is organised in lots:
open Lwt.Syntaxbelow the licence headerOption,List.concat_map,Re.execp,Sys.getenv_optandFun.idinstead of hand-written equivalentsObj.tinCometMod_main.parse_eliom_optioneref', State tables and states (the phantom types ofState.Ext.stateare now checked)eliom_params_after_action,naservice_table_content, service tablescookie_info, persistent cookies, state cookies of a requestMod_dom.select_nodeslet*instead of>>=,>|=andLwt.bind, one commit per file. The handlers ofLwt.catchkeepLwt.fail:Lwt.reraiseraises synchronously when the watched promise is already rejected (see Fix latent bugs found during the pre-release review #907).client.client.ml(pure moves)Client_relink,Client_historyandClient_call, each with an interface;Clientgoes from 2249 to 1488 lines and its interface is unchangedCommon.get_session_infointo named helpersState.Extto its own file (it uses functions ofState, which must export it, so a separate module would create a cycle) and moving the ppxCmomodule (it depends on the global settings ofPpx_eliom_utils, which uses it)querySelectorAll,compareDocumentPosition,document.createEvent,addEventListeneror the History API, and the Internet Explorer 8 workarounds (CHANGES entry).String.starts_withandString.ends_with.The public, documented API is not changed.
Testing: every commit builds,
dune build @runtestpasses, and the Ocsigen Start template (os_template) built against this branch was driven in a browser (wasm and JS clients): client-side navigation, back and forward, reload, RPC with a session reference, persistent reference, and Comet notifications between two browsers.