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

Add clj-kondo config for e/for-by #69

Closed

Conversation

simon-katz
Copy link

No description provided.

dustingetz and others added 30 commits May 18, 2023 07:42
datomic-m setup is broken with this addition, commented out from
startup, fix once needed
Local edits are stored as if on a branch. Clicking commit merges changes
back into the global transactor. Clicking discard throws the local
changes away.
With multiple editors double-clicking on 1 todo item would open the item
for editing in all editors. This fix opens the item for editing only in
the 1 editor we clicked it.

The multi-editor setup was broken anyway because
- focus could end up in the wrong editor
- edits weren't propagating to other editors
We expect to have users of ui4/button and don't want to force inline
styles on them.
Our new Datomic demos reading off the tx queue create a singleton reader
of the queue since multiple readers steal values off the queue (it is
not pub/sub). This singleton is a separate thread and we need to make
sure it doesn't block the JVM from shutting down (i.e. make them
daemon).

The TodoMVC demos now use daemon threads. The advanced todolist demos
are removed from the requires since

- they need updating to use the daemon thread pattern
- are not linked to anyway since they are in early prototype stage
… in callbacks

instead of (e/offload #(db-query a b c))
now write  (e/offload-latest db-query a b c)

differences:
- takes *electric* varargs instead of clojure thunk
- doesn't return intermediate Pendings. This fixes over-rendering and
  cycles in callbacks because work-skipping will correctly kick in.

Might not be the final solution to this problem space, but a fine
solution for now as it fixes all user concerns and doesn't need
e/snapshot.
document weird behavior in client/server transfer where we seem to see
an old value. Not in all cases though, 3 out of 4 patterns.
```
;; Was ok:
(parse-class "a b") := ["a" "b"]
(parse-class ["a" "b"]) := ["a" "b"]

;; Was failing - now fixed
(parse-class ["a b" "c"]) := ["a b" "c"] ; "a b" is not a valid dom class (contains a space)
```
For cross browser compat, some DOM attributes should always be set using
`setAttribute`, not by their corresponding property¹. E.g.

```javascript
node.setAttribute("colspan", 2); // no browser quirks
node.colSpan = 2;                // potential browser quirks
```

The Google Closure Library lists these attributes in `goog.dom.DIRECT_ATTRIBUTE_MAP_`².
We respect this requirement and implemented a similar logic to the one in
`goog.dom`, which given a `colspan` property will call
`setAttribute("colSpan")`. We later introduced a bug when we added SVG support
which requires the newer `setAttributeNS` method. `setAttributeNS`
differentiates between `colspan` vs `colSpan`, where the older `setAttribute`
doesn't. Thus, not setting the right attribute.

¹: https://github.com/google/closure-library/blob/c31b49fecdbea74df02049358460deda64bff244/closure/goog/dom/dom.js#L519
²:

```javascript
/**
 * Map of attributes that should be set using
 * element.setAttribute(key, val) instead of element[key] = val.  Used
 * by goog.dom.setProperties.
 *
 * @Private {!Object<string, string>}
 * @const
 */
goog.dom.DIRECT_ATTRIBUTE_MAP_ = {
  'cellpadding': 'cellPadding',
  'cellspacing': 'cellSpacing',
  'colspan': 'colSpan',
  'frameborder': 'frameBorder',
  'height': 'height',
  'maxlength': 'maxLength',
  'nonce': 'nonce',
  'role': 'role',
  'rowspan': 'rowSpan',
  'type': 'type',
  'usemap': 'useMap',
  'valign': 'vAlign',
  'width': 'width'
};
```
- allows keywords and symbols
- returns only unique values
- allows `nil` punning
- fails fast on other values
ggeoffrey and others added 21 commits January 23, 2024 16:25
Clicking two links fast (a few milliseconds between two clicks) would trigger a
hard navigation instead of being intercepted by the router and triggering a soft
nav.
…tab wakeup

Browsers will send a WS PING frame on tab wakeup to check if the socket is still
open. We forgot to answer pings with pongs.
A tab can wake up when:
- The user focused it after it's been in the background for a while,
- The laptop lid is opened
- User interact with the computer after the screen was sleeping (even if the
computer itself was not sleeping)
Cleaner design. HTTPKit doesn't support it, so we emulate it.
This was a chaotic situation where both users and us would get confused.
This got us so confused it triggered multiple meetings.

Solution: detect the situation and orient the user about:
- what is happening,
- what to do if this happens in prod.

We also kill the ws connection and inform the electric client to NOT attempt to
reconnect to avoid a broken program reconnection loop.
Dev code that shouldn't have been shipped
Electric client was trying to reconnect after 100ms, 100ms, 200ms, 300ms, ...
This sometimes trigger a browser safety feature in dev mode after a few fast
page refresh.

I decided to drop the first 100ms connection attempt to save on reconnection
attempts, without making the initial reconnection attempt slower.
No red errors
No stack traces
Explicit messages
@dustingetz
Copy link
Member

CA received, thank you @simon-katz
Per our DMs, the approach taken is that Kondo uses a mock/fake macroexpansion before linting. The capability has been available in Kondo for a couple years.

@simon-katz
Copy link
Author

simon-katz commented Mar 20, 2024

A few more details…

The clj-kondo feature is documented at https://github.com/clj-kondo/clj-kondo/blob/master/doc/hooks.md.

I've used the simple approach of a :macroexpand hook. This works pretty well, but one linting message is less than ideal: when e/for-by is called with the wrong number of arguments, the message is (for example) clojure.core/for is called with 1 arg but expects 2 [invalid-arity] — the mention of clojure.core/for is not ideal.

Something more sophisticated is probably possible using an :analyze-call hook.

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.