search

squint-cljs / cherry

Experimental ClojureScript to ES6 module compiler
https://squint-cljs.github.io/cherry
553 stars 22 forks source link

Support kebab-case namespace alias #71

Closed borkdude closed 1 year ago

borkdude commented 1 year ago

Discussed in https://github.com/squint-cljs/cherry/discussions/70

Originally posted by **alexdao3** November 5, 2022 I think this is a bug, as I don't see any reason why this wouldn't be supported ``` ;; cljs ["firebase-admin$default" :as fb-admin] ``` emits ``` ;; mjs import fb-admin from 'firebase-admin'; ``` `fb-admin` is invalid as a Javascript variable due to hyphens Error: `SyntaxError: Unexpected token '-'`
borkdude commented 1 year ago

@alexdao3 The logic for this lives near:

https://github.com/squint-cljs/cherry/blob/a09ebcc09344bc83cf0f533fe64a227251084bf3/src/cherry/compiler.cljc#L305

Please also add a test for this.

alexdao3 commented 1 year ago

@borkdude In addition to the above example, should this work support the following cases as well?

(:require
  ["./local_file.mjs" :as local-file] ;; a local file import
  ["url" :as some-url] ;; a JS library not containing the`"$default"` suffix
  [clojure.core :as clojure-core]) ;; Clojure namespaces?
borkdude commented 1 year ago

I think so yes

borkdude commented 1 year ago

@alexdao3 I pushed both your cherry + compiler-common PRs here:

https://github.com/squint-cljs/cherry/pull/75

It has failing tests

You can run these tests locally with:

node lib/cherry_tests.js
borkdude commented 1 year ago

I have resolved the failing test and will merge manually

borkdude commented 1 year ago

@alexdao3 Merged, thanks. I forgot to mention you as a co-author, I'll do this next time, sorry for this.

I hope to come up with some more contributor friendly way to contribute to both compiler-common and this project so tests run in CI. If you have any ideas, feel free to share them.

alexdao3 commented 1 year ago

@borkdude No worries at all, and thanks a ton for getting that over the finish line!

I'm looking at some of the changes you added on top of the PR and that would've taken me a long time to figure out 😅 Do you mind if I ask some clarifying comments on the PR? Would help me with understanding for future contributions

I hope to come up with some more contributor friendly way to contribute to both compiler-common and this project so tests run in CI. If you have any ideas, feel free to share them.

Gotcha, I'll keep that in mind!

alexdao3 commented 1 year ago

A few things I didn't quite undersatnd:

borkdude commented 1 year ago

What caused all of the failed tests with the Invalid regular expression: missing / errors?

This happens when symbol is called with an empty string namespace, which was probably triggering this.

What is the purpose of cc/target and binding it to :cherry?

In compiler-common, the code differs for squint and cherry, so this is a way to check what we are compiling for

I couldn't seem to figure out how to get bb dev setup working with the compiler-common package

Yes, just clone compiler-common within the squint or cherry clone such that you get this:

squint/compiler-common
cherry/compiler-common