Another bunch of fixes and Clojure stuff

[LeXofLeviafan]

Made several fixes for sequence functions, including making them work on all supported kinds of sequences. Also implemented a few more clojure.core things – sequence functions, syntax macroes, parsing of #inst, and #{} (based on JS Set, so I changed the name to identity-set).

[chr15m]

This is amazing. I am completely unqualified to review this work as it's way above my pay grade. I guess I'll just merge it? 😂 🙏

Perhaps after a grace period of a couple of days during which somebody smarter than me like @rcarmo or @Gozala will have a chance to review.

[zot]

Great! Nice to see Wisp getting some love!

[vendethiel]

no test for that?

[LeXofLeviafan]

I haven't found a single test for existing macros, so I didn't add any for new ones. Though I guess it wouldn't hurt adding tests for wisp.expander…

[chr15m]

@LeXofLeviafan now that this has had review I'm happy to pull the trigger on this one whenever you are ready. Up to you whether you want to address those other two things but I don't see why they should block this PR going in.

[LeXofLeviafan]

@chr15m The third thing is more of a question, so it certainly doesn't block anything (methinks) :-)
I was wondering if I should alias identity-set to set ((def set identity-set)) and revert parsing #{} to set() so that if someone uses #{} currently it'd be easier to migrate without name clashing (just define their set() implementation after Wisp imports) but then it'd still be incompatible with conj etc…

[chr15m]

@LeXofLeviafan which option will cause the least disruption to existing codebases? Or are they about the same? I'm strongly in favour of backwards compatibility and not breaking people's existing software.

[LeXofLeviafan]

@chr15m originally #{} was parsed/transpiled as set(); so if anyone uses it now it's by providing a set() function (which isn't present in Wisp) so that their own implementation of a set is used. Since the Wisp set I implemented isn't exactly on par with Clojure set (as it's comparing arrays/objects by identity as opposed to by value), I named it identity-set to keep the distinction visible, and thus changed the parser output to produce identitySet() from #{}.
And now I'm thinking if I should change that back and make set an alias to identity-set that can be replaced by alternate implementation; on the other hand, a literal syntax is the only thing anyone would gain from doing so, since sequence functions expect specifically an identity-set. (Though maybe that can be somewhat mitigated by changing them to support JS Set instead like = does now… Then, any object that is recognized as a Set could be a valid input – though the output would still be an identity-set.)

[chr15m]

Thank you for the further explanation.

@LeXofLeviafan if I do (console.log [1 2 3]) what I get back is a native JS Array. If I do (console.log {:a 12}) I get back a native JS Object like {"a": 12}. Should it not be the case that (console.log #{1 2 3}) gives us a native JS Set? So e.g. something like set=(...s)=>new Set(s);.

I think one of the features that sets Wisp apart from ClojureScript is that you get the native JS types by default without having to use the #js reader macro.

There is probably a great deal of complication I am missing here, so thanks for your patience!

[LeXofLeviafan]

@chr15m The identity-set that I implemented is basically an extended JS Set. It has all the Set methods and is recognized by instanceof Set check as a Set, so the only difference in practice should be that it's callable like the Clojure set is (so (#{1 2 3} x) is a valid membership test).

[chr15m]

Ok gotcha.

I haven't checked but I think if Wisp allows you to do the same alias changing thing with vector and hash-map then we should support the set alias too. If not, I reckon don't worry about it and just go with your identity-set as per the PR. Either way it's better than the current situation of missing set!

With regards to supporting incoming JS Set in the place of identity-set, that sounds to me like something the user would expect from Wisp. Would be cool. I'm happy to merge without that though.

[LeXofLeviafan]

@chr15m Well, both vectors and dictionaries are currently parsed as literals, but quoted lists ('(1 2 3)) and empty lists (()) are both converted to a list() call, so technically it's possible to do a substitution with that one (though list is not a native type). …Come to think of it, that means list literals have the same issue as set literals (constructing function must be defined/imported in scope for the evaluation to work).

[chr15m]

High wizardry!

[rcarmo]

Curious to try this out when it’s pushed to npm.

…

On 1 Dec 2019, at 01:25, Chris McCormick ***@***.***> wrote:  High wizardry! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[LeXofLeviafan]

…Incidentally, all discussed changes (except for macros tests) have been implemented, so unless you want some other changes to be made (or for someone to review last two commits) I'd consider this pull-request completed.

[chr15m]

Thank you, will merge today and put out a new release on npm.

[rcarmo]

Great news. Should we start thinking about more kinds of automated tests here too?

…

On 3 Dec 2019, at 21:25, Chris McCormick ***@***.***> wrote:  Thank you, will merge today and put out a new release on npm. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[chr15m]

If by "start thinking about" you mean "submitting PRs implementing" then yes. ;)

[chr15m]

@LeXofLeviafan I have merged your PR into master on my local, done a make clean and then make test and it's throwing the following error:

node ./bin/wisp.js ./test/test.wisp 
/home/chrism/dev/wisp/sequence.js:351
            throw TypeError('' + 'Can not seq ' + sequence);
            ^

TypeError: Can not seq [object Object]
    at /home/chrism/dev/wisp/sequence.js:351:19
    at seq (/home/chrism/dev/wisp/sequence.js:352:11)
    at first (/home/chrism/dev/wisp/sequence.js:209:211)
    at ensureDictionary (/home/chrism/dev/wisp/sequence.js:303:23)
    at /home/chrism/dev/wisp/sequence.js:146:26
    at Array.map (<anonymous>)
    at /home/chrism/dev/wisp/sequence.js:145:31
    at mapv (/home/chrism/dev/wisp/sequence.js:150:11)
    at conj (/home/chrism/dev/wisp/sequence.js:307:312)
    at /home/chrism/dev/wisp/wisp.js:88:20
make: *** [test1] Error 1

The tests are passing on this PR on Travis though. I've verified my local version of Node is the same as the one running CI. Any ideas?

[LeXofLeviafan]

@chr15m There's no yarn.lock file (nor its npm equivalent) in the repository, so it could be that your dependencies aren't matching? One issue that I encountered while working on this is that the Wisp transpiler used by make doesn't expand macros correctly unless they've been implemented/fixed in the node_modules version of Wisp…

[chr15m]

@LeXofLeviafan thanks for the pointer, this was the issue. 👍

[Gozala]

I just had being getting bunch of notifications from wisp repo lately & wanted to thank you all for the joy I’ve being experiencing seeing it improved.

I also want to share why originally #{...} was compiling to set(...) (not sure if it’s helpful, but it won’t hurt) - I used to use wisp in multiple JS environments one had native JS Set while other did not, I also happened to have a piece of code that needed sets that I wanted to share across two environments & compilation to set(...) allowed me to provide implementation per necessary without having to add runtime that compiled wisp would carry. Although in retrospect Set polifyll might have being good enough.

As of lists intentions were the same “no runtime” for compiled programs (In my experience lists were only used by the compiler & macros).

[chr15m]

The "no runtime" thing is fantastic. Something I've been having fun with lately is compiling bits of ClojureScript UI to vanilla JS using Wisp and then adding a sprinkle of JS shims to patch over missing native functions. With this technique I've been able to make artifacts as small as 1.6k and declarative UIs with Preact in 10k. Great thing about this is I still get to use the wonderful ClojureScript tooling (like shadow-cljs) during development.

An example is here. This 10k artifact compiled with Wisp runs this declarative UI: https://svgflipbook.com/account and the same thing compiles to > 100k using ClojureScript tooling. 10x difference in size!

Thank you so much for making Wisp. 🙏