Fork me on GitHub
#figwheel-main
<
2018-08-31
>
onetom03:08:01

what's the point of using :as if the original namespace is the same as the alias, like [router5 :as router5]?

pesterhazy07:08:03

Excellent question, I saw that somewhere and copied the style but didn’t verify that’s different from just [router5]

onetom07:08:52

I hope it's not different, because that would suggest some kind of unintuitive magic going on behind the scenes.

pesterhazy08:08:30

you're right, it seems equivalent

onetom03:09:59

yeah, that "makes sense" 🙂 knowing how es6 works.

pesterhazy08:09:21

honestly I haven't fully grokked that part. Why do you need require("foo").default, and sometimes not?

onetom08:08:32

I fixed up my figwheel.main+devcards example project. It works as expected now. https://github.com/onetom/clj-figwheel-main-devcards

pesterhazy09:08:53

I created an issue to clarify my recent adv-comp type annotation question: https://github.com/clojure/clojurescript-site/issues/260

pesterhazy11:08:26

Running into an issue with the test runner. On this branch: https://github.com/pesterhazy/cljs-spa-example/pull/11/files, when I open the page initially, it looks like no tests are registered. When you reload (or clear the cache), that doesn't help. But when you touch the cljs file containing the tests, they get registered properly

bhauman11:08:54

@pesterhazy you need to use the async support in cljs.test as well or there is no test finished signal

pesterhazy11:08:01

@bhauman I don't think that's it. The async test is commented out, plus I reimplemented what cljs.test/async does in the promise-test fn: https://github.com/pesterhazy/cljs-spa-example/pull/11/files#diff-26c35a846286dd881684332ac2e68d68R6

bhauman11:08:54

@pesterhazy you have clojure.test/IAsyncTest not cljs.test/IAsyncTest

pesterhazy11:08:25

clojure.test is an alias for cljs.test, so they should be equivalent, no?

pesterhazy11:08:07

ah I found a way to reliably repro the issue

bhauman11:08:10

hmmm I don’t know what your experiencing

pesterhazy11:08:49

- start/dev - see the test show up - change the name of a test, arithmetic-test-expected-to-fail -> arithmetic-test-expected-to-fail-2 - the reload

pesterhazy11:08:55

expected result: you see the new test

pesterhazy11:08:09

actual result: the renamed test isn't found

pesterhazy11:08:27

until you Shift-Reload

pesterhazy11:08:36

so it must be related to browser caching...

bhauman11:08:36

ok that means one thing then

bhauman11:08:51

the test runner isn’t being reloaded

bhauman12:08:07

or its cached somehow

pesterhazy12:08:43

when I restart the figwheel process, that doesn't have any effect

bhauman12:08:17

let me look at the example

bhauman12:08:25

Look in the console and see if when you change the test file, if the test runner is reloaded

pesterhazy12:08:18

I can even run scripts/dev --reset, and it's still empty unless I shift-reload

bhauman12:08:48

first remove cljs-test-display from the mixture for now

bhauman12:08:06

as we want to know whats happening

bhauman12:08:44

is the runner getting reloaded?

bhauman12:08:48

not the tests

bhauman12:08:04

the runner has all the tests compiled into it

pesterhazy12:08:33

not sure I understand the question. Tto repro the issue I'm clicking the Reload button in the browser, so fighweel-reloading isn't involved

bhauman12:08:35

the runner is an insane macro that has all the code in it

pesterhazy12:08:57

i'll try with just clojure.test/run-tests yeah?

bhauman12:08:43

yes that sounds great

pesterhazy12:08:55

removed the reference to cljs-test-display and push my changes

pesterhazy12:08:02

same problem

pesterhazy12:08:35

again what I did was to change the name of the single test, then reloaded

bhauman12:08:55

well that is expected

pesterhazy12:08:59

once I shift-reload, it works again

bhauman12:08:32

first why are you manually reloading?

bhauman12:08:49

I think I’m not getting all the details

pesterhazy12:08:43

the first time I ran into this, I was manually reloading because I wasn't sure if some change I made was taking effect

pesterhazy12:08:00

based on the assumption that a manual Reload usually fixes things

pesterhazy12:08:20

in this case, though, after renaming the test figwheel's live reloading gave the correct result

pesterhazy12:08:39

but manually reloading didn't - which is what was confusing me

bhauman12:08:57

OK well that sounds exactly like caching

pesterhazy12:08:02

my thinking is, if it's fixed by Shift-Reload, it must be caching, no?

pesterhazy12:08:25

and specifically Chrome's cache (not anything in Figwheel's internal state)

bhauman12:08:43

as long as, it gives you the same result as the first time you open the tab?

pesterhazy12:08:17

not sure I understand that question

bhauman12:08:24

I’m assuming caching caches on page load

bhauman12:08:53

and I’m wondering when you have these cache hit situations if you are getting the same result you get when the page first loads

bhauman12:08:47

One thing that needs to be remembered here is that the run-tests macro is super funky

bhauman12:08:28

if you look at the compiled test runner you will see a list of all the tests that it was able to find at compile time

bhauman12:08:55

embbeded in a bunch of macro generated code to run them

bhauman12:08:06

its kinda nasty

bhauman12:08:23

and there you go

pesterhazy12:08:26

that's what you see when you reload

pesterhazy12:08:43

core_test.js is reloaded properly (I can verify if I look at the file)

bhauman12:08:45

yep its cached

pesterhazy12:08:46

but test_runner.js isn't

bhauman12:08:04

you are using the figwheel server?

pesterhazy12:08:23

which, as you say, contains references to the tests function names in cljs-spa.core-test because of the run-tests macro

bhauman12:08:16

ok this makes sense

bhauman12:08:49

and its partially chromes fault

bhauman12:08:52

its caching the artifact at some time z, and you are getting that cached artfact no matter what’s been hot reloaded

bhauman12:08:49

co.deps.ring-etag-middleware

pesterhazy12:08:50

I think it must be the If-Modified-Since: Fri, 31 Aug 2018 12:11:04 GMT Request Header

pesterhazy12:08:32

If I right this correctly, Chrome is not sending an Etag in the request

pesterhazy12:08:02

does that middleware take into account the If-Modified-Since header?

bhauman12:08:12

even the initial serving of the file should contain an etag

pesterhazy12:08:03

in the case, though, I think the problem lies with the Last-Modified response header the server sends back

pesterhazy12:08:11

my hypothesis is some component uses the last-modified time of test-runner.cljs to determine the LastModified header of the response of test_runner.js

pesterhazy12:08:57

but that's not necessarily correct - the js output should change if the source cljs file gets modified, but also wgeb any of the dependencies is modified

bhauman12:08:03

something to know is that the time of modification of the input cljs files and ouput js files is kept in sync

bhauman12:08:46

@danielcompton you should be included in this conversation

bhauman12:08:24

Well this is enough information to track this down. It does seem like excluding last modified headers makes sense if we are including etags

bhauman12:08:00

but I’d have to defer to @danielcompton on this because he has put a lot of thought in to how to make this behave correctly

bhauman12:08:24

thanks for taking the time to track this down

pesterhazy13:08:07

Just tried in Chrome Canary, regular Chrome and Firefox - all incorrectly get a 304 Not modified

pesterhazy13:08:55

Do you want me to file an issue?

bhauman13:08:50

the issue I believe should go on the middleware

bhauman13:08:27

just because that’s where we are handling these things

pesterhazy13:08:30

@bhauman looks like the figwheel server uses 2 different middlewares, not-modified and etag. If I'm not mistaken, the faulty header is generated by the not-modified wrapper, not the etag one

pesterhazy13:08:21

even though the If-None-Match header sent by Chrome is out of date, the server still returns a 304 because the If-Modified-Since request header matches the file's last-modified date

pesterhazy13:08:05

one solution would be to remove the not-modified middleware, or to strip out the Last-Modified header from the response (it's not needed because the response already contains the ETag)

bhauman13:08:14

is the hot reload why the last modified header is the same? and the the browser is fetching something older than the hot reloaded code from its cache?

pesterhazy13:08:18

the Last-Modified response header matches the mtime of the cljs file

bhauman13:08:33

it didn’t change

pesterhazy13:08:42

right, only the macro expansion changed

bhauman13:08:47

the runner doesn’t change

bhauman13:08:30

this stuff keeps raising its head in strange ways

pesterhazy13:08:42

yeah, caching is the worst

bhauman13:08:54

hmmm so what are the downsides of removing the last-modified?

bhauman13:08:06

I’m going to go look at the code

bhauman13:08:50

thats interesting

bhauman13:08:21

@pesterhazy ok the not-modified middleware doesn’t add the last modified head but it does return the 304

pesterhazy13:08:48

I wonder if that or is correct

bhauman13:08:06

its a good question

pesterhazy13:08:16

if the Etag doesn't match, it still considers the resource not changed

bhauman13:08:25

yeah that seems wrong

bhauman13:08:14

yeah that is wrong

pesterhazy13:08:38

there are 3 cases, Request-has-Etag and matches, Request-has-Etag and mismatches, and Request-has-no-Etag

bhauman13:08:38

if an etag is provided and supplied and they don’t match it should fail right there

pesterhazy13:08:54

in the 2nd case, the response should be considered modified

pesterhazy13:08:10

I'll submit a PR to the middleware

bhauman13:08:34

there are the cases where no etag is provided by the response

bhauman13:08:02

actually thats covered by your cases

bhauman13:08:16

OMG yet another caching bug

bhauman13:08:30

this shit has been happening for years

bhauman13:08:48

well the good news is that the etag approach is correct and that the not-modified code is at fault

bhauman13:08:36

@pesterhazy if he isn’t responsive, the code is simple enough to include in figwheel

bhauman13:08:12

we could even hot patch it in the near term as its a very stable codebase

pesterhazy13:08:13

>>> A recipient must ignore If-Modified-Since if the request contains an If-None-Match header field; the condition in If-None-Match is considered to be a more accurate replacement for the condition in If-Modified-Since, and the two are only combined for the sake of interoperating with older intermediaries that might not implement If-None-Match.

bhauman13:08:37

Fantastic!

pesterhazy13:08:50

re: the case where both headers are specified

pesterhazy13:08:59

from RFC 7232

bhauman13:08:05

so the logic should be 304 if etags are provided by request and response, and they don’t match

bhauman13:08:47

I was afraid of that

pesterhazy13:08:11

that's good news, isn't it? that's already in ring master

bhauman13:08:23

absolutely great news

bhauman13:08:47

I read the commit wrong and thought he introduced it

bhauman13:08:56

yes and that will involve upgrading the jetty server as well

pesterhazy13:08:32

can confirm that adding

ring/ring-core {:mvn/version "1.7.0-RC2"}
to deps.edn fixes the issue

bhauman13:08:03

thanks for getting to the bottom of it

pesterhazy13:08:22

thanks for helping me understand this

bhauman13:08:40

I’m wondering how to approach this for figwheel-main

bhauman13:08:09

I could bump ring to 1.7.0-RC2

pesterhazy13:08:24

what's the downside of doing that?

bhauman13:08:35

but that will change the jetty server deps

bhauman13:08:57

which is not the end of the world

bhauman13:08:10

but it could cause folks some headaches, because jetty is in datomic cloud

bhauman13:08:38

ah I have to write a doc about jetty conflicts anyway

pesterhazy13:08:48

ah I didn't know that jetty had changed so much

bhauman13:08:40

what happens is jetty is broken in to many many parts and the all have the save version number

bhauman13:08:14

different projects only require certain parts and when you put them together, bam

bhauman14:08:08

yeah its a trade off, let me look at the commits of 0.1.7-RC2 and see if any prs are really relevant

bhauman14:08:27

but I think its probably the best idea

bhauman14:08:37

to just go with it 0.17-RC2

dottedmag14:08:26

@bhauman So Jetty guys have botched the rule "unit of reuse is unit of release" and chopped it thinner than it should be?

bhauman14:08:17

yeah, but jetty is freaking huge

bhauman14:08:25

so I understand

bhauman14:08:06

and if the versions were different across the whole release it would drive someone insane

dottedmag14:08:05

I wonder if one can do something like Conflicts: jetty-something (!= 1.2.3) (in maven?) so that these errors surface immediately.

dottedmag14:08:37

So that all parts of jetty would conflict with other parts of jetty with non-matching version.

bhauman14:08:26

they kinda do

bhauman14:08:41

by accident

bhauman14:08:06

since the cross requires throw missing dependency errors most of the time

bhauman14:08:43

actually I take that back

bhauman14:08:00

most of the time it does just fail on a missing method somewhere

danielcompton22:08:26

So is everything sorted here now?

danielcompton22:08:08

Might be useful if it hasn’t been mentioned