Fork me on GitHub
#clojurescript
<
2021-11-08
>
borkdude13:11:24

Is there a valid reason fixtures with test-vars only work after you execute test-all-vars or is this a bug?

$ plk
ClojureScript 1.10.597
cljs.user=>  (require '[clojure.test :as t])
nil
cljs.user=>  (t/deftest foo [])
#'cljs.user/foo
cljs.user=> (t/use-fixtures :each {:before (fn [] (prn :before))})
#'cljs.user/cljs-test-each-fixtures
cljs.user=> (t/test-vars [#'foo])
nil
cljs.user=> (t/test-all-vars 'cljs.user)
:before
nil
cljs.user=> (t/test-vars [#'foo])
:before
nil

dnolen15:11:13

@borkdude would have to look at the details to determine if there is a good reason

dnolen15:11:42

@tkjone what happens when you add this other firebase package as a dependency?

athomasoriginal15:11:22

@dnolen How do you mean? Like, in the package.json?

dnolen15:11:48

yes, @thheller said there were two packages

borkdude15:11:50

@dnolen I suspect it has to do with macro vs fn. macros have the opportunity to invoke the CLJS analyzer whereas a fn doesn't

dnolen15:11:53

@borkdude I don't know - I looked quickly - might just be a bug - test-vars doesn't install the fixtures the way tets-all-vars does

athomasoriginal15:11:52

TMK I believe thheller was referring to the packages inside of the @firebase node_module. So, there is no second package you can add to the package.json, it’s just firebase and then that comes with all the modules.

athomasoriginal15:11:04

This is where firebase is kind of odd. You install firebase (via npm or yarn) and it comes with all the sub packages e.g.

node_modules
  ...
  @firebase/
    auth/
    app/
    firestore/
    auth-compat/
    ...

borkdude15:11:28

I noticed that yes. But test-vars is a function. test-all-vars is a macro which calls another macro which uses the CLJs analyzer,. https://github.com/clojure/clojurescript/blob/6443518850d8e4f1e09be99d4669e8d4a5e893a3/src/main/cljs/cljs/test.cljc#L343

dnolen15:11:15

ah yeah, could maybe put a macro inside test-vars to run the boilerplate side-effects?

dnolen15:11:13

@tkjone oh I see it kind of a "bad" package

dnolen15:11:25

it declares a name, but does not that use that name for the primary import

dnolen15:11:01

@tkjone if this is a sufficiently common thing the answer is probably simple

athomasoriginal15:11:03

yes! 🙏 (much better articulation than me)

dnolen15:11:33

one thing I don't understand is why @firebase/auth fails though?

dnolen15:11:55

i.e. this workaround stuff ... ok - but the actual root cause

dnolen15:11:31

or is that you cannot require @firebase/auth from JS fundamentally - because it doesn't exist?

athomasoriginal15:11:05

You can require @firebase/auth and it will return the ESM module. So it’s found.

athomasoriginal15:11:18

The issue seems to be that some of the the deps of @firebase/auth are not resolved. One of those deps exxecute some side effects. Because those side-effects never execute, we get an error.

dnolen15:11:56

@tkjone but the thing is did you try this in a non-ClojureScript context i.e. does it fail in exactly the same way?

dnolen15:11:21

i.e. CommonJS require of this package, process Webpack/Rollup - it should fail in the same way

dnolen15:11:35

if it doesn't fail in the same way then maybe something missing

dnolen15:11:58

(though hard to see what - because all this stuff is intentionally pushed outside of ClojureScript)

athomasoriginal15:11:44

> RE: did you try this in a non-ClojureScript context Yes. the demo repo is here https://github.com/athomasoriginal/firebase-cljs/tree/firebase-method-4

athomasoriginal15:11:55

Is the src dir you will see a deps.js and index.js file which should be setup in the same way, just with JS.

athomasoriginal15:11:45

does that align with what you were thinking?

dnolen16:11:50

@tkjone you have no description of what happens in this case, or the others in the README

athomasoriginal16:11:27

One moment. I will update. But for now: the issue is that when you call getAuth it will throw an error in the console saying Error: Component auth has not been registered yet

dnolen16:11:47

@tkjone but the point is that this is broken w/ JS as well

dnolen16:11:09

and there's no issue in the firebase repo itself?

athomasoriginal16:11:00

well, it’s broken because of the use of require("@firebase/auth") instead of the firebase recommended require('firebase/auth')

dnolen16:11:17

hrm - but it's very strange because you can in fact require @firebase/auth

1
dnolen16:11:49

and there's a redirection that was pointed out

dnolen16:11:11

@tkjone https://www.npmjs.com/package/firebase why isn't this one scoped? can you not use the unscoped one?

athomasoriginal16:11:11

How do you mean? TMK I am using the uncscoped package? Or do you mean use the require the unscoped package from CLJS?

dnolen16:11:27

so even after npm -i firebase you get @firebase in your node_modules ?

dnolen16:11:31

hrm, I'm honestly confused as to how that works

dnolen16:11:59

does Node.js consistently allow for @foo/bar.js and foo/bar.js ?

dnolen16:11:29

if this is consistent, can probably fix this in a simple way

athomasoriginal16:11:06

Same. I am stumped how to google this in general, because it seems so “unique”.

lilactown16:11:41

I think that the firebase package has dependencies on the namespaced @firebase/... packages

lilactown16:11:08

which is why you get all of the @firebase/.. packages installed when you run npm i firebase. theyre transitive

lilactown16:11:53

@tkjone have you tried importing the un-namespaced npm package, i.e. firebase/auth no @?

athomasoriginal17:11:52

Yeah, npm install firebase/auth will fail.

lilactown17:11:59

your require

lilactown17:11:20

(:require
  ["firebase/auth" :as auth])

athomasoriginal17:11:29

in CLJS? yes. That throws in CLJS

lilactown17:11:58

ah that's the bug ticket that dnolen created i see

1
lilactown17:11:37

the bug actually doesn't make sense to me yet

lilactown17:11:24

here's what I did:

$ mkdir firebase-test && cd firebase-test
$ npm init
$ npm i firebase
$ ls node_modules/firebase/auth
cordova      dist         package.json react-native

lilactown17:11:57

so firebase/auth exists on disk. the description of the bug makes me think that the issue is that it doesn't exist on disk

lilactown17:11:09

firebase/auth is a subdirectory of the firebase package. is that the real issue here?

dnolen17:11:54

@lilactown that might be it - we only index the top level

dnolen17:11:48

but indexing more than the top-level is potentially problematic because of Node.js dep graphs - which is why we don't do it

lilactown17:11:48

it's a pretty common thing in npm packages these days to have one big package and then instruct people to import some package/dir/subdir

dnolen17:11:16

oh but in this case this isn't a nested node_modules

1
dnolen17:11:36

@lilactown but how is this lower dir resolved?

dnolen17:11:51

(i.e. it is not relative)

lilactown17:11:50

not sure what you mean by "it is not relative" but, the way I understand it is:

import { baz } from 'package/foo/bar'
will: 1. look up package in node_modules 2. search for the foo directory in node_modules/package. there might be some misdirection from package.json that I don't understand here but IME it just finds a directory in the package 3. search for bar inside the foo directory

dnolen17:11:14

@lilactown you missed my point above

dnolen17:11:25

@foo/bar vs. foo/bar

dnolen17:11:45

does Node.js have resolution magic for scoped packages

lilactown17:11:52

yes AFAICT there are slightly different resolutions between scoped packages and non-scoped packages and what's on disk

lilactown17:11:23

scoped:

node_modules/
  @firebase/
    package/
      package.json
non-scoped:
node_modules/
  firebase/
    package/
      ...
    package.json

dnolen17:11:41

but the question is

dnolen17:11:51

does firebase/auth magically check @firebase/auth

lilactown17:11:10

no. you can look at it in the firebase repo

dnolen17:11:31

then we're going in circles 🙂

dnolen17:11:59

or are you saying

dnolen17:11:05

we need to index @firebase/packages

dnolen17:11:18

because Node.js looks in packages

lilactown17:11:08

I'm not sure what the solution is because I still don't know what the problem is yet. why can't @tkjone use:

(:require ["firebase/auth" :as auth])
?

athomasoriginal17:11:51

I suspect you know this happens, but when I try that I get a Compile Exception

No such namespace: firebase/app, could not locate firebase_SLASH_app.cljs, firebase_SLASH_app.cljc, or JavaScript source providing "firebase/app" (Please check that namespaces with dashes use underscores in the ClojureScript file name) in file src/demo/firebase_webpack.cljs

lilactown17:11:57

yeah sounds like dnolen knows why now

👍 1
dnolen17:11:25

it doesn't work because we didn't index that one

lilactown17:11:27

here's what I would expect that to resolve to on disk:

in my example instructions above:
$ cat node_modules/firebase/auth/dist/index.esm.js
export * from '@firebase/auth';
//# sourceMappingURL=index.esm.js.map

dnolen17:11:35

we indexed @firebase and @firebase/auth works

dnolen17:11:36

(at least wrt. resolution, it fails because of other reasons and that can be avoided w/ firebase/auth except that doesn't work because ClojureScript didn't index it)

lilactown17:11:48

so it sounds like from that description, we do not index any subdirectories of packages

dnolen17:11:23

hrm but you don't need to

dnolen17:11:34

I looked at this by installing firebase and my theory is there is just magic

dnolen17:11:56

Node.js magically resolves firebase/auth even though it is located in @firebase/auth

lilactown17:11:11

😄 it's not magic

lilactown17:11:28

I'm 99% certain that node.js resolves it to firebase/auth/dist/index.cjs.js

lilactown17:11:33

and that requires @firebase/auth

dnolen17:11:10

wow, I see it installed a separate package

dnolen17:11:06

ok, yeah so the problem might be that we just don't index firebase as multiple packages

dnolen18:11:25

@tkjone I think this is just a resolution issue - allowing you specify something is not desirable

dnolen18:11:47

I've marked the issue as a blocker

dnolen18:11:11

@tkjone have to set this aside for now but I reproduced the issue exactly https://github.com/clojure/clojurescript/pull/110

dnolen18:11:51

looking at the index dump - it's clear that firebase is malformed - I don't think this is a particularly hard issue - but need to make sure it doesn't affect what's already there

dnolen18:11:23

once resolved can cut a release just for this

❤️ 1
dnolen18:11:48

as to why it was reported before I suspect that firebase may have restructured the library

dnolen18:11:18

I see that they leverage exports in package.json and I don't see that we handle that

olive18:11:20

for re-frame apps, is there a difference in performance characteristics between these two approaches?

parent component/
├─ child component/
│  ├─ grandchild component/
│  │  ├─ @(rf/subscribe [:stuff-grandchild-needs])
│  ├─ child content
├─ parent-content
parent component/
├─ prop = @(rf/subscribe [:stuff-grandchild-needs])
├─ child component [prop]/
│  ├─ grandchild component [prop]/
│  │  ├─ [:div prop]
│  ├─ child content
├─ parent-content
assume each component is its own function, not inline. in plain english, does what level you put a subscription in the component hierarchy make a performance difference? having subscriptions littered throughout the component hierarchy makes devcards difficult without mocking rf/subscribe for all of its calls throughout the component tree

athomasoriginal19:11:52

Not in and of itself, no. What matters is how many times everything has to render + size of each component (do they perform a lot of logic, or have a lot of HTML etc). This is why, you could have a nested structure and it can run quickly as long as things don’t need to update frequently. Ultimately, the only way to know for sure is you have to test everything.

athomasoriginal19:11:53

My rule of thumb is I don’t like to work with deeply nested components because they become tricky to maintain. So, I start by optimizing for ease of development, then I work towards improving render performance if needed.

lilactown19:11:10

when react (& therefore reagent/re-frame) re-renders a component, it will re-render everything below it, too. what that means is it will do some equality checks to see if its props have changed, and if they have it will re-run the function to produce new VDOM nodes. all of this can take time

lilactown19:11:49

you can tune the performance of your application by moving subscriptions to different levels. subscriptions at the leaves tend to cause less calculations during render. however, your subscriptions can also be expensive - and depending on the shape and performance of your subscription graph, you might find that having a top-level subscription actually provides more performance than many subscriptions in the leaves

lilactown19:11:13

at the end of the day, you should profile your application to see where your performance problems lay

lilactown19:11:37

(I’m adding more detail to the correct info/advice of athomasoriginal)

athomasoriginal19:11:51

Agree with everything lilactown said 🙇

olive19:11:47

some frontend languages/frameworks (e.g. elm) are “smart enough” to discern that even though the arguments to a given function (i.e. props for a component) may change, the only DOM changes are at the leaves where the prop is actually being used in rendering, so re-render is limited to that grandchild leaf node dependent, which to me is the biggest performance concern because rerenders also affects app behavior (you lose hover states when a component rerenders, for example). is a re-frame/reagent webapp in that class of frameworks/libraries that can discern the smallest possible DOM update needed, or does it naively rerender the whole component tree when a prop changes?

athomasoriginal19:11:10

I have never seen it happen for the reasons you described.

lilactown19:11:15

yes at a couple of different layers

lilactown19:11:42

reagent/re-frame use React. so you get all the behaviors & benefits of React, plus some other stuff

athomasoriginal19:11:06

When I have seen it happen? When I am absolutely obliterating the front end with chart data, user input, async calls etc. I suppose i’m saying I would have to try to get the effect your talking about 😉 But we’re talking in hypotheticals right now so it’s tough to cover all the fun scenarios.

lilactown19:11:52

React maintains a virtual representation of what’s currently on the page (called a virtual DOM, VDOM for short) and when your components “render,” what they’re actually doing is creating a new representation of this VDOM. React then diffs the new and old VDOM and determines what changes to make to the real DOM.

lilactown19:11:30

When React diffs the VDOM and then changes the DOM, they call this “committing” the latest VDOM

lilactown19:11:47

this makes it so that if a piece DOM hasn’t actually changed, you can maintain DOM state like hover or whatever like you said

lilactown19:11:29

and we can write our components in the sort of immediate-mode style as if they are going to change everything

lilactown19:11:48

so there’s this diffing and committing phase which can help save us some. however, this can be expensive, running all of the component functions to render the new VDOM and then diffing it. so React (& Reagent) also provide the ability to bail out of rendering by memoizing components, so that if they receive the same props in the same place in the tree, it will just reuse the last subtree rather than running the function for that component again and creating a new VDOM.

lilactown19:11:38

Reagent memoizes all of your components by default by comparing all props you pass in using Clojure equality =. this does a deep equality check of nested structures like maps, vectors, etc.

lilactown19:11:42

sometimes this deep equality check can take too much time, at which point you want to start moving when and where new data is generated.

lilactown19:11:18

very often, it’s fine to just naively generate new VDOM on every change, and then start optimizing. React gives you all the tools you need here. reagent makes it slightly easier at the expense of slightly more complexity

olive20:11:53

thanks so much for both of your explanations!

athomasoriginal19:11:56

@dnolen I think I might have identified this issue a while ago https://gist.github.com/athomasoriginal/fd00691bde0be8d9a52c33f3ca968bdc - the reason, as you noted, the scoped solution worked is because version 8 of the library was structured differently from the current version 9. Also, as I see now, the way I was looking at the problem was wrong.

dnolen19:11:15

@tkjone yes if firebase had been failing this whole time we would have heard about it long ago

Andy Carlile20:11:12

in F# there is a method of accessing server logic from the client called https://github.com/Zaid-Ajaj/Fable.Remoting. What's your preferred method in clj/cljs?

isak21:11:07

The closest to that approach may be this: https://github.com/ptaoussanis/sente

isak21:11:36

Though I think most people just use regular HTTP requests for most things.