Clojurians
#cljs-dev
<
2017-07-12
>

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

darwin00:07:10

so we will rely on ad-hoc curl for now

mfikes00:07:26

that works :slightly_smiling_face:

juhoteperi00:07:04

How about just clj-http instead of shelling to curl? :slightly_smiling_face:

mfikes00:07:08

I guess the tokens might end up in ps output. Maybe there is a way to prevent that as well with some fancy curl thing

juhoteperi00:07:54

Will help with tokens not being in command options and thus ps also

darwin00:07:24

security for those tokens is not critical IMO, the worst case scenario is flooding travis with requests and they have their own protection (quotas I believe)

darwin00:07:31

@mfikes btw. I realized we cannot revoke those tokens via github as you expected, travis gem tool just uses github for auth, but generates own token and deletes the github personal token

mfikes00:07:56

Ahh. Yeah, since the GitHub repos are public, anyone can read them.

darwin00:07:40

not really, those personal tokens are visible only in user’s account, they are not related to repos AFAIK

mfikes00:07:03

It would be interesting if you wanted to revoke the Travis API key (if someone was maliciously using it to trigger builds). Hmm.

darwin00:07:44

they simply don’t want to create persistent github tokens, they ask github for auth and then believe their own tokens (I guess)

darwin00:07:19

do you see any personal github tokens here? https://github.com/settings/security

mfikes00:07:30

@darwin Here is my recent stuff of interest

darwin00:07:11

@mfikes hmm, have you used the travis token --org for generating the token?

mfikes00:07:49

And when I authenticated, prior to that step, I passed it a GitHub personal access token

mfikes00:07:22

Of course, I’m like that dog typing at the computer

darwin00:07:49

I didn’t create the github token by hand, it just wanted my github login credentials

darwin00:07:54

anyways, I think we ended up with the same end result, we had a token generated by travis system, and not anymore connected to github system

darwin00:07:25

if you revoked you personal github token, I belive the travis token would still work, unless travis watches for this kind of event

mfikes00:07:31

Right. Maybe the Travis API key has an indefinite lifetime

olivergeorge04:07:38

@mikethompson I have no authority at all related to this but does it sound like something you'd be interested in contributing?

anmonteiro06:07:11

got excited and spent the evening figuring out how to emit Node requires for indexed modules https://dev.clojure.org/jira/browse/CLJS-2213

anmonteiro06:07:15

feedback welcome

thheller08:07:17

@anmonteiro I think it is better to create pseudo namespaces as I outlined here: https://dev.clojure.org/jira/browse/CLJS-2061

thheller08:07:17

if you def into each namespace there need to be extra checks for clashes with user defs

thheller08:07:44

AFAICT (:require [react]) would clash with (def react "foo")

juhoteperi10:07:42

@thheller Even with pseudo namespaces, wouldn't [react :as react] also clash with (def react ...)?

juhoteperi10:07:50

And the generated name in JS code, can be something else than ns name

thheller10:07:28

not if you use (react/createElement ...) or so since the (def react ...) will not blow away the actual React reference

thheller10:07:26

but yes it may conflict since you are allowed to call the js modules directly

juhoteperi10:07:44

Not sure what @anmonteiro patch does, but @dnolen example was to emit e.g. var localReact = require("react");, where localReact wouldn't clash with def react?

thheller10:07:55

(:require [clojure.string :as str]) + (str "foo") is different for CLJS imports

thheller10:07:13

I honestly don’t know why we allow using the :as directly

thheller10:07:46

IMHO should have something like ES6 default exports or so instead

juhoteperi10:07:09

Do you refer to using :as directly with modules that only export single function?

juhoteperi10:07:12

Something to thing about I guess

thheller10:07:41

It is probably too late to change that now but it really isn’t that bad

juhoteperi10:07:44

We have now pretty much only looked into supporting CommonJS, but would be good to check how ES6 works

thheller10:07:04

default import/export

thheller10:07:48

but not really a big deal, we should just warn about possible clashes

thheller10:07:11

we already do for things like (ns my.app) (def react ...) and (ns my.app.react)

thheller10:07:14

I just did the pseudo namespace thing since it means that we will have ONE require("react") in the project non one per ns

thheller10:07:39

but since not many namespaces will be importing "react" thats really not an issue either

thheller10:07:42

still surprised that this is suddenly under consideration at all again

dnolen11:07:14

António’s patch looks fine though might need a refactor when someone decides to tackle CLJS-2214, I’m not really concerned about the clash problem, I’m going to add a small change to prepend “node$module$” to the munged lib name

mfikes11:07:24

@dnolen I’m working on a subsequent minor aget patch that exposes the :checked-arrays to self-hosted, and uses a dyn var just like other options (:static-fns)

dnolen11:07:13

@mfikes cool! I read over your notes seems OK at first glance - we should sort out this first compile issue

mfikes11:07:35

@dnolen Yes, that is on my list.

dnolen11:07:24

the only real concern I have with António’s patch is Figwheel Node.js support since we’re dropping the goog.addDependency stuff again. When he comes back online we should check that it’s necessary and revert otherwise

dnolen12:07:38

@mfikes one thing about your patch is I do think you want runtime warnings with checked-aget since there’s a lot of stuff inference will not catch

mfikes12:07:11

Oh, maybe do the stuff :pre does, but write to stderr at runtime?

dnolen12:07:59

yeah that’s what I was trying to suggest sorry if that wasn’t clear

mfikes12:07:13

Yep, that’s completely clear. There is no precedent I can look to on how to do this I suspect. Will try it and figure it out. I guess it can check to see if *print-err-fn* is bound to a value before attempting to write out.

mfikes12:07:10

Actually, check to make sure it is not the default one that throws if not bound, or something like that

dnolen12:07:33

@mfikes hrm goog.debug.getStacktrace is a thing by the way

mfikes12:07:04

Oh, warn with some context?

dnolen12:07:11

I think showing the whole stack is probably not necessary - but showing the name of the offender is neccessary

mfikes12:07:10

Hah, the initial compile problem is solved now. Perhaps as a result to the switch to ana/*checked-arrays*

mfikes12:07:06

Lot of abuse, I guess

mfikes12:07:27

Some of that stuff is being repeated by different macroexpansions? Hmm

dnolen12:07:22

as far as how to print I think maybe we should use goog.log and forego our own printing machinery here?

dnolen12:07:55

@mfikes I think that’s just macro stuff

mfikes12:07:14

I can try goog.log right now in React Native / Figwheel and see where the errors go

mfikes12:07:27

For React Native, using goog.log puts its warnings at the same place that other warnings go, which might be considered good

mfikes12:07:39

(.warning goog.log (.getLogger goog.log "") "hello")

mfikes12:07:21

^ This is the bottom of the iOS simulator, which is where other things get logged by React Native

mfikes12:07:16

And, you can also include an optional exception, which the user can use the disclosure triangle to see.

kommen12:07:59

would there be interest to support webworker as a target for clojurescript next to :nodejs? currently a custom bootstrap script is needed to make it work with optimizations :none: https://github.com/bhauman/lein-figwheel/wiki/Using-Figwheel-with-Web-Workers

dnolen12:07:04

@mfikes yes this seems fine to me, we probably want to due a little bit of diligence and check that the DCE is good here for the trivial hello world program w/ these new deps

richiardiandrea12:07:06

not saying it is not going to work, but logging is a delicate topic, because cljs-devtools might break

dnolen12:07:19

@richiardiandrea why would it break?

richiardiandrea12:07:51

good question, I don't know...I should have stayed silent :smile:

dnolen12:07:28

@kommen go for it

mfikes12:07:51

If you include the opt_exception argument, it doesn’t actually show up.

dnolen12:07:37

@mfikes other option is to just call console.warn (need to test it exists)

dnolen12:07:00

the real value here is that you would source mapped trace

mfikes12:07:20

^ That stacktrace is not the one I wanted. Perhaps it flows through too much RN machinery.

dnolen12:07:31

if we can make RN nice that’s cool - but I’m really more concerned about Browser/Node.js cases first

mfikes12:07:45

Yeah. RN is no big deal.

mfikes12:07:07

console.warn results in the same behavior as the above

dnolen12:07:25

we should check in browser though

mfikes12:07:27

I unfortunately don’t have a conventional Browser project set up to try warnings in

mfikes12:07:42

I’ll go through Quick Start

dnolen12:07:43

I think source mapping will apply to logged Error?

dnolen12:07:06

@mfikes just fire up a webserver via browser REPL :slightly_smiling_face:

rauh12:07:15

You get source mapped errors with: (js/console.warn "Bad array access in: %o", (js/Error. "stack"))

rauh12:07:32

console.war is IE >= 8. So pretty safe

dnolen12:07:37

yes that’s what I was thinking

dnolen12:07:57

@rauh we still need to check because we might be running somewhere else

dnolen12:07:04

not browser, not Node

mfikes13:07:01

So maybe some variation on

(defn- checked-aget
  ([array idx]
   (try
     (checked-aget' array idx)
     (catch :default e
       (js/console.warn e))))
  ([array idx & idxs]
   (try
     (apply checked-aget' array idx idxs)
     (catch :default e
       (js/console.warn e)))))

dnolen13:07:09

@mfikes heh we don’t want hard errors, just log :slightly_smiling_face:

dnolen13:07:30

exact same as logic as :pre but not a throwing assertion - just a warning

mfikes13:07:48

Yep. Too early in the morning :slightly_smiling_face:

dnolen13:07:40

@mfikes probably want a private helper maybe-warn or something that tries with console.warn when available

dnolen13:07:55

later we can think about something more general - but that would be good enough for first cut

mfikes13:07:40

@dnolen I’m going to be out for a large chunk of the day today, but I’ve attached a patch to https://dev.clojure.org/jira/browse/CLJS-2198 that does the “log on bad invoke with :warn” set. It could perhaps be cleaned up with exactly what and how it logs, but the essence of the idea is there now.

mfikes13:07:06

If you happen to choose to merge it in so it can be subsequently revised, I don’t see any issue with that.

dnolen13:07:20

@mfikes great thanks! will review, if it’s mostly there yeah that’s what I’ll do :slightly_smiling_face:

dnolen13:07:30

thanks for all the work on this - was a long one :smile:

mfikes13:07:58

Yeah, thank you too. I’m starting to think the patch is pretty bad-ass now, compared to what we started with.

dnolen14:07:11

Given the array stuff is still under review and the Node.js stuff happened quicker than expected I’m thinking António’s post could go tomorrow?

dnolen14:07:47

I’m also wondering where :global-exports should fall? I think that’s another big thing to cover - it’s really a standalone major enhancement to foreign libs to make their usage more idiomatic

juhoteperi14:07:15

Will António’s post cover Node.js/global-exports, or do we write about those separately?

dnolen14:07:30

yeah that’s what I’m asking

dnolen14:07:14

I don’t think folding it into :npm-deps / Node.js post makes much sense - it’s really an à la carte feature

juhoteperi14:07:40

Probably true

juhoteperi14:07:56

The post for global-exports can link and mention the other post

dnolen14:07:50

I’m more than happy to contribute another post for :global-exports

dnolen14:07:06

but also won’t complain if somebody else wants to take it :slightly_smiling_face:

juhoteperi14:07:31

It is okay for me if you write it

juhoteperi14:07:29

I'm working on :global-exports patch now

juhoteperi15:07:25

emit_global_requires_test.core.node$module$react = window.React;
emit_global_requires_test.core.node$module$react_dom$server = window.ReactDOMServer;

juhoteperi15:07:07

We should probably use something like this instead of window to look for the global from proper place:

var g;
    if (typeof window !== "undefined") {
      g = window;
    } else if (typeof global !== "undefined") {
      g = global;
    } else if (typeof self !== "undefined") {
      g = self;
    } else {
      g = this;
    }

juhoteperi15:07:05

Can I def this g somewhere (with proper name) and use that, or should I create a function that can be called instead of directly accessing window? Function would probably be eliminated if :global-exports is not used.

juhoteperi15:07:32

Should I use munge-node-lib to name these locals, or create another function to name these a bit different, and modify all the places where munge-node-lib is used to check for :global-exports?

anmonteiro15:07:03

@dnolen we can't put node libs in cljs_deps.js because they're not CLJS deps anymore

anmonteiro15:07:22

Otherwise goog will try to load them via their mechanisms

anmonteiro15:07:04

I don't think it's a problem in Figwheel because it doesn't need to be aware of these?

anmonteiro15:07:16

Node will know how to load

anmonteiro15:07:08

Also, I'll tweak the post in the next hour or so to account for the new patch so that it can go out today

dnolen15:07:46

@juhoteperi goog.global is a thing :wink:

juhoteperi15:07:05

Aha! Nice :slightly_smiling_face:

dnolen15:07:15

@anmonteiro the problem is that Figwheel uses the dep graph to figure out what to reload

dnolen15:07:22

@bhauman could provide some context here

anmonteiro15:07:26

I still don't get the problem. If I did what I think I did, we're only filtering out stuff that are closure / cljs libs

anmonteiro15:07:51

^ the other way round... filtering out node modules

anmonteiro15:07:27

So Node will know how to load those. Figwheel will still know how to handle everything else

anmonteiro15:07:55

Those modules wouldn't really get reloaded much anyway because of require.cache

dnolen15:07:00

I could be wrong - I’m just saying that was the exact location where the churn occurred

dnolen15:07:34

previously we also filtered out Node.js stuff there - and it broke Figwheel

dnolen15:07:39

for Node.js usecases

dnolen15:07:09

oh but your change does less I guess that’s what you’re saying :slightly_smiling_face:

dnolen15:07:17

so maybe - yeah that’s OK

anmonteiro15:07:21

@dnolen what Node stuff did we filter out?

anmonteiro15:07:47

The things we're filtering out here are not Closure namespaces, which is why I think it's not a problem

anmonteiro15:07:34

Yeah I didn't revert the fix that was in place

dnolen15:07:26

oh k, pardon the noise then :slightly_smiling_face:

dnolen15:07:15

@anmonteiro let me know when your post is ready, I think it might be nice to have a catchier title on this one

dnolen15:07:40

it would be cool to get some HN/Reddit love on this

anmonteiro15:07:46

@dnolen open for suggestions

dnolen15:07:07

@anmonteiro prep the post at your leisure, and let me ponder

anmonteiro15:07:51

I genuinely think we're introducing the concept well enough for that to happen

anmonteiro15:07:04

I could be wrong, there's no predicting the Internet

anmonteiro15:07:04

I saw your comment about renames w/ JS modules, btw

anmonteiro15:07:18

That's a good point, I'll open a ticket and provide a patch

kommen15:07:18

@dnolen created https://dev.clojure.org/jira/browse/CLJS-2216, happy to take feedback, it’s my first patch submission

dnolen15:07:40

@kommen you have your CA sorted?

dnolen15:07:00

@anmonteiro cool re: patch

dnolen15:07:15

@kommen ok, not sure when I’ll have a chance to look at - patch is appreciated

dnolen15:07:24

but we’re trying to flush our release queue right now

juhoteperi15:07:58

Okay, got reagent working with global-exports: reagenttest.testreagent.node$module$create_react_class = goog.global.createReactClass;

juhoteperi15:07:02

What should I do with the left part of the name? Keep node$module$, separate name for global-exports or rename node$module$ so it makes sense to the same name with both node and global-exports?

juhoteperi15:07:42

Using same name for both Node require and global-exports keeps the diff much smaller

juhoteperi16:07:06

Well, here is the first version, quite simple as it works similar to Node requires: https://dev.clojure.org/jira/secure/attachment/17052/CLJS-2214.patch

anmonteiro16:07:39

I'm really excited about string requires that work in Node.js. I think Lumo will be able to take advantage of it seamlessly

anmonteiro16:07:12

Where cljs.analyzer/node-module-dep? is just a wrapper around require.resolve

anmonteiro16:07:35

Dont even need to index node modules since target is always Node :tada:

juhoteperi16:07:05

All these changes together should make using Reagent (and Om probably) much easier in more exotic envs, like React-native

anmonteiro16:07:57

You should mention that in your post

juhoteperi16:07:59

No need to create mock cljsjs.react files anymore to get Reagent to compile

juhoteperi16:07:24

Hmm, we should now check what is the order requires are resolved (and document this), currently I think, 1. Node require, if target is node and module is found in node_modules 2. Global export, if foreign-lib provides the name and has global-exports entry 3. Cljs/Closure namespace (includes processed JS modules) 4. Foreign-lib

juhoteperi16:07:15

Previously it was common to replace Cljsjs foreign-libs with local Cljs namespaces, but will that be needed in future?

juhoteperi16:07:44

I guess the Cljsjs packages should be excluded anyway (if something else is used) so no files get added, and then order doesn't matter much

dnolen16:07:32

agree about mentioning that in the post

dnolen16:07:12

@juhoteperi keeping the same name will make debugging harder though - they should be different

dnolen16:07:18

global$module$ please

dnolen16:07:21

left feedback on the ticket

bhauman16:07:15

@dnolen @anmonteiro as long as cljs_deps.js still contains dependency graph information for all the users cljs namespaces this shouldn't be a problem for Figwheel. It would be dissapointing if after finally getting it fixed if it broke again.

bhauman16:07:46

but from reading the code it looks ok as long as user cljs namespaces are not in the module index

dnolen16:07:54

@bhauman yeah we’re not going to release if it’s broken again :slightly_smiling_face:

bhauman16:07:01

thanks :slightly_smiling_face:

bhauman16:07:18

the code looks good though

anmonteiro16:07:24

@bhauman module index is stuff like react, react-dom/server, etc

anmonteiro16:07:35

only things that we js/require

bhauman16:07:40

yeah that stuff is never reloaded

anmonteiro16:07:49

great to have your confirmation

anmonteiro16:07:25

@dnolen just pushed an update to the post draft

anmonteiro16:07:54

my tentative title is now: > Symbiosis with JavaScript modules: Improvements to JS Module Processing

dnolen16:07:33

hehe a bit too technical, busy w/ something for a moment, will be back w/ feedback and some ideas in the PR

anmonteiro16:07:51

I’ll think of something else

dnolen17:07:59

@anmonteiro I left a title suggestion in PR, more tweaks, and I think we need to mention :npm-deps

dnolen17:07:13

it’s kind of an obscure thing since we really needed all this other stuff for it to be compelling

anmonteiro17:07:30

I’ll rework that part

anmonteiro17:07:35

done with the tweaks now

anmonteiro17:07:02

@dnolen I like the title idea. Added a paragraph about :npm-deps, direct link to latest commit: https://github.com/clojure/clojurescript-site/pull/104/commits/332fbd85060aae5317c15d14d213da93bf5f2245

anmonteiro17:07:09

@dnolen when you’re ready to push the merge button, let me know so I can squash commits before

dnolen17:07:00

@anmonteiro I added my last few tweaks - this looks great

dnolen17:07:18

go ahead and squash your commits and I can do a final review

anmonteiro17:07:22

@dnolen thanks for the thorough review

juhoteperi17:07:35

@dnolen Did you have some ideas for helper fns re 2214?

dnolen17:07:23

I mean there’s just code duping here - let’s just lift and make the decision in the helper

dnolen17:07:32

later if we need to make it more general we can and it will be behind an fn

dnolen17:07:16

@anmonteiro there’s still double first

dnolen17:07:20

in the first para

anmonteiro17:07:37

initial release?

anmonteiro17:07:04

force pushed, you may have to hard refresh

dnolen17:07:39

@anmonteiro you ready to ship? :smile:

anmonteiro17:07:53

@dnolen click the button

anmonteiro17:07:01

green means go :white_check_mark:

dnolen17:07:27

done, it’s take around 3-5 minutes to appear in my experience

dnolen17:07:51

when it hits I’ll post it to HN/Reddit

dnolen17:07:16

@anmonteiro if you see it first ping me - doing another thing for a moment

anmonteiro17:07:26

I can keep an eye out

anmonteiro17:07:00

renders very nicely too

anmonteiro17:07:09

I feel like I got headings right :stuck_out_tongue:

juhoteperi17:07:50

@dnolen https://dev.clojure.org/jira/secure/attachment/17053/CLJS-2214-2.patch fixed the name but didn't lift any code to functions

dnolen17:07:18

that’s fine, can do separately, I’ll take a look later and see if there’s some easy refactoring to do

juhoteperi17:07:21

I have hard time deciding what would be good way to share the code without duplicating the checks in resolve-var and new function

dnolen17:07:22

doesn’t need to be in the patch

dnolen17:07:29

k, I might agree

dnolen17:07:31

we’ll see

dnolen17:07:05

post is submitted to HN & Reddit, feel free to upvote in the new section of HN

dnolen17:07:00

you shouldn’t upvote from direct links

dnolen17:07:05

will get pushed down

dnolen17:07:12

I would delete that :wink:

dnolen17:07:37

sorry should have mentioned that earlier

dnolen17:07:50

@anmonteiro ^

anmonteiro18:07:35

I guess it prevents circle-upvoting

darwin18:07:17

what happened? I always use HN through my reader, so I guess I’ve been always upvoting from direct links…

dnolen18:07:08

early upvoting from direct links is the problem

dnolen18:07:37

you also never want to share direct links on social media

dnolen18:07:45

for things you want upvoted

darwin18:07:32

ok, good to know :slightly_smiling_face:

anmonteiro18:07:40

oh the HN cult :slightly_smiling_face:

dnolen18:07:57

it’s a game but it’s a good way to drive traffic / get visiblity

dnolen20:07:30

going to add a new print flag *print-fn-bodies* and I’m going to set this to false

dnolen20:07:40

I think a lot people have asking for fn override, but this is really all they want

juhoteperi20:07:48

Perhaps we should have mentioned React already working with adv build in the post :d

dnolen20:07:58

to see collapsed fns when they print - anybody have a problem with this?

dnolen20:07:17

@juhoteperi agreed, if @anmonteiro wants to supply tweak would merge ASAP :slightly_smiling_face:

juhoteperi20:07:07

@anmonteiro I put up Reagent site compiled with npm-deps: https://twitter.com/JuhoTeperi/status/885228578098601984

juhoteperi20:07:11

Reagent demo site is quite heavy, probably as it uses react-dom/server and enable-console-print!

anmonteiro20:07:53

Rly cool. At lunch now but will tweak soon

dnolen20:07:40

@anmonteiro or we could tweak for you we made it to HN front page

anmonteiro20:07:52

I have no issue with thtat

dnolen20:07:03

@juhoteperi would take a PR that notes that this works with link to Reagent project

anmonteiro20:07:21

We’re actually on the front page right now :slightly_smiling_face:

dnolen20:07:43

yeah :slightly_smiling_face:

dnolen20:07:48

somebody out there loves us!

dnolen20:07:05

I’m not going to link to the HN thread, but we are there on the middle of the page

dnolen20:07:27

not sure how many questions we’ll get but feel free to jump in and answer if you have time

juhoteperi20:07:40

Hmm what would be good place to add these links on the blog post

juhoteperi20:07:07

At the end, or maybe under "This is a big deal"?

anmonteiro20:07:57

I’m back if you haven’t done it yet I can do it

juhoteperi20:07:32

You can link to this PR instead of the branch: https://github.com/reagent-project/reagent/pull/306

juhoteperi20:07:16

Looking and source-map-explorer, why isn't module processed JS included there? Is Closure code included at all in source-maps?

dnolen20:07:44

@juhoteperi cannot parse that sentence :slightly_smiling_face:

anmonteiro20:07:01

I know what he’s talking about

anmonteiro20:07:14

somehow Closure JS doesn’t seem to appear in the source map explorer viz

anmonteiro20:07:26

like goog. stuff is not in there I think

anmonteiro20:07:34

maybe we don’t source-map those, that’s why?

dnolen20:07:46

oh yeah probably not

dnolen20:07:54

we just generate source maps for our own stuff

dnolen20:07:13

we really should be using Closure for this stuff and stop doing it ourselves

dnolen20:07:25

(outside of the :none source maps)

dnolen20:07:38

source map composition should be handled by Closure now that they support it

juhoteperi20:07:13

anmonteiro: Btw. the link points to a separate folder, the main site is not using this version yet

anmonteiro20:07:49

yeah, didn’t I link to the correct place>

dnolen20:07:37

@juhoteperi @anmonteiro is this ready to go then? yes? no?

juhoteperi20:07:40

the link is correct, I just wonder if it should more clearly say this is a test, and the main site is not using this yet

juhoteperi20:07:59

and perhaps it should clearly mention advanced optimization works with npm-deps?

dnolen21:07:46

@anmonteiro I agree with that feedback

anmonteiro21:07:00

will do in a min

dnolen21:07:18

@mfikes will need a rebased patch for CLJS-2198 given all of this excitement :slightly_smiling_face:

mfikes21:07:36

Yes @dnolen running tests on a rebase now.

dnolen21:07:42

@juhoteperi global exports patch merged

mfikes21:07:52

I’m also thinking of adding some unit test coverage to checked-aget and friends

dnolen21:07:07

@mfikes that would be nice!

dnolen22:07:25

@danielcompton even in preliminary testing this finds a lot of problems

dnolen22:07:35

it’s really too much, it’s needs to be off

dnolen22:07:09

people (really maintainers) need to try it, fix their libs and then move forward

dnolen22:07:48

@danielcompton that commit also doesn’t show the big picture, there’s a patch underway which is comprehensive

danielcompton22:07:15

Oh wow, thanks, I hadn't seen that one

danielcompton22:07:13

Reading that ticket, am I right in saying the end user effect is that you can compile more optimised array optimisations?

mfikes22:07:20

Perhaps one day in the far future we can even round the integer arguments being passed to aget and aset so they act exactly like Clojure. (Perhaps even to pull that off, there might need to be some unsavory backwards compat flag.)

mfikes22:07:44

I would say the ticket is largely about correctness

dnolen22:07:56

@mfikes I think it’s safe to do that right now if a user uses :check-arrays :error

dnolen22:07:04

if you want to do it go for it :wink:

mfikes22:07:15

Ahh, that is a valid argument, I believe :slightly_smiling_face:

dnolen22:07:30

yes that’s the beautiful thing about :error

dnolen22:07:35

you want your code to be correct

dnolen22:07:40

we can do whatever the heck we want

mfikes22:07:19

Ahh @dnolen, that would fail :check-arrays :error and then if someone built in :advanced

mfikes22:07:47

(aget #js [1] 0.5) would return 1 until :advanced was enabled

dnolen22:07:31

the behavior for :advanced + :check-arrays :error shouldn’t change here

dnolen22:07:56

if you’re developing with :check-arrays :error then your code is good - and we can do right thing for the final build

mfikes22:07:00

Agreed. Perhaps it could be pulled off with a sufficient revision of the patch in that case.

mfikes22:07:22

Sounds tempting to try :slightly_smiling_face: Hrm.

dnolen22:07:52

basically the caveat is the same as goog.assert

dnolen22:07:07

those get erased if Closure can prove they aren’t needed internally

dnolen22:07:56

@mfikes maybe we should hold of on that now that I think about it

dnolen22:07:19

so that we also erase checks if we can prove the wrong thing won’t happen

dnolen22:07:27

yeah ok let’s leave that alone for now

mfikes22:07:27

Hah! We have been piling it on so high. :slightly_smiling_face: I’m having trouble being absolutely sure things would work :slightly_smiling_face:

dnolen22:07:43

lets just wrap it up :slightly_smiling_face:

dnolen22:07:48

we can finesse later

mfikes22:07:59

People need time to address all the bad agets before you can even really do this anyway :slightly_smiling_face:

mfikes22:07:42

Hah Closure is friggin awesome. It is warning me about the JavaScript my new tests is generating:

WARNING: /Users/mfikes/Projects/clojurescript/builds/out-adv/cljs/array_access_test.js:578: WARNING - Array index out of bounds: NUMBER -1.0 578 [length: 1] [source_file: /Users/mfikes/Projects/clojurescript/builds/out-adv/cljs/array_access_test.js]
try{var values__17240__auto___20028 = (function (){var x__8744__auto__ = ([(1)][(-1)]);

mfikes23:07:38

@danielcompton The “future” thing we were mentioning above: (aget #js [3 7] 0.5) should return 3. Today, it essentially gets turned into (js* "[3,7][0.5]") and in the future, it could be turned into (js* "[3,7][0.5|0]"), thus behaving exactly like Clojure. But doing that would break abuse like (aget #js {:foo 17} "foo").

anmonteiro23:07:12

our post is getting more love than I expected

anmonteiro23:07:30

makes the work seem worthwhile :slightly_smiling_face:

dnolen23:07:07

a really amazing job from everyone all around! you all are the best! :slightly_smiling_face: