This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2017-07-12
Channels
- # aleph (10)
- # beginners (79)
- # boot (81)
- # chestnut (3)
- # cider (9)
- # cljs-dev (336)
- # cljsrn (17)
- # clojure (121)
- # clojure-boston (1)
- # clojure-italy (4)
- # clojure-nl (1)
- # clojure-russia (218)
- # clojure-spec (32)
- # clojure-uk (98)
- # clojurescript (109)
- # cloverage (1)
- # core-async (5)
- # cursive (17)
- # datascript (15)
- # datomic (38)
- # editors (4)
- # emacs (6)
- # graphql (1)
- # hoplon (140)
- # instaparse (1)
- # jobs (2)
- # klipse (1)
- # leiningen (4)
- # lumo (2)
- # mount (103)
- # off-topic (3)
- # om (8)
- # onyx (19)
- # parinfer (32)
- # pedestal (3)
- # precept (32)
- # re-frame (33)
- # reagent (24)
- # remote-jobs (11)
- # rum (1)
- # spacemacs (1)
- # specter (37)
- # unrepl (4)
- # untangled (43)
- # vim (11)
How about just clj-http
instead of shelling to curl? 🙂
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
Will help with tokens not being in command options and thus ps
also
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)
@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
not really, those personal tokens are visible only in user’s account, they are not related to repos AFAIK
It would be interesting if you wanted to revoke the Travis API key (if someone was maliciously using it to trigger builds). Hmm.
they simply don’t want to create persistent github tokens, they ask github for auth and then believe their own tokens (I guess)
do you see any personal github tokens here? https://github.com/settings/security
at least this is what I see in mine: https://box.binaryage.com/travis-token-auth.png
And when I authenticated, prior to that step, I passed it a GitHub personal access token
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
if you revoked you personal github token, I belive the travis token would still work, unless travis watches for this kind of event
got excited and spent the evening figuring out how to emit Node requires for indexed modules https://dev.clojure.org/jira/browse/CLJS-2213
feedback welcome
@anmonteiro I think it is better to create pseudo namespaces as I outlined here: https://dev.clojure.org/jira/browse/CLJS-2061
if you def
into each namespace there need to be extra checks for clashes with user defs
@thheller Even with pseudo namespaces, wouldn't [react :as react]
also clash with (def react ...)
?
And the generated name in JS code, can be something else than ns name
not if you use (react/createElement ...)
or so since the (def react ...)
will not blow away the actual React
reference
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
?
Do you refer to using :as
directly with modules that only export single function?
Something to thing about I guess
We have now pretty much only looked into supporting CommonJS, but would be good to check how ES6 works
I just did the pseudo namespace thing since it means that we will have ONE require("react")
in the project non one per ns
but since not many namespaces will be importing "react"
thats really not an issue either
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
@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)
@mfikes cool! I read over your notes seems OK at first glance - we should sort out this first compile issue
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
@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
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.
Actually, check to make sure it is not the default one that throws if not bound, or something like that
I think showing the whole stack is probably not necessary - but showing the name of the offender is neccessary
Hah, the initial compile problem is solved now. Perhaps as a result to the switch to ana/*checked-arrays*
as far as how to print I think maybe we should use goog.log
and forego our own printing machinery here?
For React Native, using goog.log puts its warnings at the same place that other warnings go, which might be considered good
^ This is the bottom of the iOS simulator, which is where other things get logged by React Native
And, you can also include an optional exception, which the user can use the disclosure triangle to see.
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
@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
not saying it is not going to work, but logging is a delicate topic, because cljs-devtools
might break
@richiardiandrea why would it break?
good question, I don't know...I should have stayed silent 😄
^ That stacktrace is not the one I wanted. Perhaps it flows through too much RN machinery.
if we can make RN nice that’s cool - but I’m really more concerned about Browser/Node.js cases first
You get source mapped errors with: (js/console.warn "Bad array access in: %o", (js/Error. "stack"))
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)))))
@mfikes probably want a private helper maybe-warn
or something that tries with console.warn
when available
later we can think about something more general - but that would be good enough for first cut
@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.
If you happen to choose to merge it in so it can be subsequently revised, I don’t see any issue with that.
Yeah, thank you too. I’m starting to think the patch is pretty bad-ass now, compared to what we started with.
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?
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
Will António’s post cover Node.js/global-exports, or do we write about those separately?
I don’t think folding it into :npm-deps
/ Node.js post makes much sense - it’s really an à la carte feature
Probably true
The post for global-exports can link and mention the other post
It is okay for me if you write it
I'm working on :global-exports
patch now
emit_global_requires_test.core.node$module$react = window.React;
emit_global_requires_test.core.node$module$react_dom$server = window.ReactDOMServer;
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;
}
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.
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
?
@dnolen we can't put node libs in cljs_deps.js because they're not CLJS deps anymore
Otherwise goog will try to load them via their mechanisms
I don't think it's a problem in Figwheel because it doesn't need to be aware of these?
Node will know how to load
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
@juhoteperi goog.global
is a thing 😉
Aha! Nice 🙂
@anmonteiro the problem is that Figwheel uses the dep graph to figure out what to reload
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
^ the other way round... filtering out node modules
So Node will know how to load those. Figwheel will still know how to handle everything else
Those modules wouldn't really get reloaded much anyway because of require.cache
I could be wrong - I’m just saying that was the exact location where the churn occurred
@dnolen what Node stuff did we filter out?
The things we're filtering out here are not Closure namespaces, which is why I think it's not a problem
Yeah I didn't revert the fix that was in place
@anmonteiro let me know when your post is ready, I think it might be nice to have a catchier title on this one
@dnolen open for suggestions
Agreed
@anmonteiro prep the post at your leisure, and let me ponder
I genuinely think we're introducing the concept well enough for that to happen
I could be wrong, there's no predicting the Internet
I saw your comment about renames w/ JS modules, btw
That's a good point, I'll open a ticket and provide a patch
@dnolen created https://dev.clojure.org/jira/browse/CLJS-2216, happy to take feedback, it’s my first patch submission
@anmonteiro cool re: patch
Okay, got reagent working with global-exports: reagenttest.testreagent.node$module$create_react_class = goog.global.createReactClass;
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?
Using same name for both Node require and global-exports keeps the diff much smaller
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
I'm really excited about string requires that work in Node.js. I think Lumo will be able to take advantage of it seamlessly
Where cljs.analyzer/node-module-dep?
is just a wrapper around require.resolve
Dont even need to index node modules since target is always Node 🎉
All these changes together should make using Reagent (and Om probably) much easier in more exotic envs, like React-native
You should mention that in your post
No need to create mock cljsjs.react
files anymore to get Reagent to compile
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
Previously it was common to replace Cljsjs foreign-libs with local Cljs namespaces, but will that be needed in future?
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
@juhoteperi keeping the same name will make debugging harder though - they should be different
@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.
but from reading the code it looks ok as long as user cljs namespaces are not in the module index
@bhauman module index is stuff like react
, react-dom/server
, etc
only things that we js/require
great to have your confirmation
@dnolen just pushed an update to the post draft
my tentative title is now: > Symbiosis with JavaScript modules: Improvements to JS Module Processing
hehe a bit too technical, busy w/ something for a moment, will be back w/ feedback and some ideas in the PR
I’ll think of something else
@anmonteiro I left a title suggestion in PR, more tweaks, and I think we need to mention :npm-deps
it’s kind of an obscure thing since we really needed all this other stuff for it to be compelling
I’ll rework that part
done with the tweaks now
@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
@dnolen when you’re ready to push the merge button, let me know so I can squash commits before
@anmonteiro I added my last few tweaks - this looks great
@dnolen thanks for the thorough review
@dnolen Did you have some ideas for helper fns re 2214?
I mean there’s just code duping here - let’s just lift and make the decision in the helper
@anmonteiro there’s still double first
initial release?
fixed
force pushed, you may have to hard refresh
@anmonteiro you ready to ship? 😄
@dnolen click the button
green means go ✅
@anmonteiro if you see it first ping me - doing another thing for a moment
I can keep an eye out
@dnolen https://clojurescript.org/news/2017-07-12-clojurescript-is-not-an-island-integrating-node-modules
renders very nicely too
I feel like I got headings right 😛
@dnolen https://dev.clojure.org/jira/secure/attachment/17053/CLJS-2214-2.patch fixed the name but didn't lift any code to functions
that’s fine, can do separately, I’ll take a look later and see if there’s some easy refactoring to do
I have hard time deciding what would be good way to share the code without duplicating the checks in resolve-var
and new function
wow..
I guess it prevents circle-upvoting
what happened? I always use HN through my reader, so I guess I’ve been always upvoting from direct links…
oh the HN cult 🙂
Perhaps we should have mentioned React already working with adv build in the post :d
@juhoteperi agreed, if @anmonteiro wants to supply tweak would merge ASAP 🙂
@anmonteiro I put up Reagent site compiled with npm-deps: https://twitter.com/JuhoTeperi/status/885228578098601984
Reagent demo site is quite heavy, probably as it uses react-dom/server and enable-console-print!
Rly cool. At lunch now but will tweak soon
@anmonteiro or we could tweak for you we made it to HN front page
I have no issue with thtat
@juhoteperi would take a PR that notes that this works with link to Reagent project
We’re actually on the front page right now 🙂
not sure how many questions we’ll get but feel free to jump in and answer if you have time
Hmm what would be good place to add these links on the blog post
At the end, or maybe under "This is a big deal"?
I’m back if you haven’t done it yet I can do it
Go ahead
You can link to this PR instead of the branch: https://github.com/reagent-project/reagent/pull/306
anmonteiro: Btw. the link points to a separate folder, the main site is not using this version yet
yeah, didn’t I link to the correct place>
@juhoteperi @anmonteiro is this ready to go then? yes? no?
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
and perhaps it should clearly mention advanced optimization works with npm-deps?
(and React)
@anmonteiro I agree with that feedback
will do in a min
@dnolen @juhoteperi OK new diff https://github.com/clojure/clojurescript-site/pull/114
Looking and source-map-explorer, why isn't module processed JS included there? Is Closure code included at all in source-maps?
@juhoteperi cannot parse that sentence 🙂
I know what he’s talking about
somehow Closure JS doesn’t seem to appear in the source map explorer viz
like goog.
stuff is not in there I think
maybe we don’t source-map those, that’s why?
anmonteiro: Btw. the link points to a separate folder, the main site is not using this version yet
@juhoteperi global exports patch merged
With https://github.com/clojure/clojurescript/commit/3b5d88a2546c07fc27da5014445f03ac15a9f22f, why do the warnings default to off? Performance implications?
@danielcompton even in preliminary testing this finds a lot of problems
@danielcompton that commit also doesn’t show the big picture, there’s a patch underway which is comprehensive
if you’re curious https://dev.clojure.org/jira/browse/CLJS-2198
Oh wow, thanks, I hadn't seen that one
Reading that ticket, am I right in saying the end user effect is that you can compile more optimised array optimisations?
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.)
Ahh @dnolen, that would fail :check-arrays :error
and then if someone built in :advanced
if you’re developing with :check-arrays :error
then your code is good - and we can do right thing for the final build
Agreed. Perhaps it could be pulled off with a sufficient revision of the patch in that case.
Hah! We have been piling it on so high. 🙂 I’m having trouble being absolutely sure things would work 🙂
People need time to address all the bad aget
s before you can even really do this anyway 🙂
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)]);
@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")
.
our post is getting more love than I expected
makes the work seem worthwhile 🙂