Skip to content

Conversation

@alexdao3
Copy link
Contributor

@alexdao3 alexdao3 commented Nov 7, 2022

Fixes #71

@alexdao3 alexdao3 force-pushed the ad/add-support-kebab-case-aliases branch 3 times, most recently from da87af0 to b294d41 Compare November 7, 2022 03:41
@alexdao3
Copy link
Contributor Author

alexdao3 commented Nov 7, 2022

Converting to DRAFT PR because this isn't quite ready yet. I just encountered the below

;; cljs
(:require ["firebase-admin$default" :as fb-admin])

(.createUser (fb-admin/auth) #js {:uid uid})
;; generated js
import {  } from 'firebase-admin'
import { auth as fb-admin_auth, firestore as fb-admin_firestore } from 'firebase-admin$default'
import fb_admin from 'firebase-admin';

Note it generates fb-admin_auth

[p & _props] (when suffix
(str/split suffix #"\."))]
(str/split suffix #"\."))
snake-case-as (some-> as str (str/replace "-" "_"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function we use in the compiler to do this transformation is called munge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip!

@alexdao3 alexdao3 force-pushed the ad/add-support-kebab-case-aliases branch from b294d41 to 4f4aa9e Compare November 7, 2022 22:40
@alexdao3
Copy link
Contributor Author

alexdao3 commented Nov 7, 2022

@borkdude Looks like a couple of the functions I touched e.g. process-require-clause, (defmethod emit-special 'ns), etc might have gotten moved to the common compiler / removed entirely. Should I rebase and make relevant updates in the common compiler before requesting a review?

Also I wasn't entirely clear on the expected JS output for [clojure.core :as clojure-core] (https://github.com/squint-cljs/cherry/pull/72/files#diff-23674a1b2d812522c7730bef4ea308884bcfa3ffea3e264ba406eb618e698ffdR345-R347) so this test is currently failing. Let me know if you have any suggestions!

@borkdude
Copy link
Member

borkdude commented Nov 7, 2022

@alexdao3 I'm sorry for that! I'm still in progress of moving the common code to compiler-common and did a big chunk of that today.

Previously I used compiler-common as a git submodule, but I had to revert that because it could no longer be used as a git dependency in deps.edn (deps.edn doesn't support submodules).

What I moved to is the following: in development I work with a local checkout of compiler-common within the squint (and cherry) project and there is a task bb bump-common which then inserts the git sha from compiler-common into the deps.edn. It's a little bit involved but I didn't know any better way to do it.

But I do realize that this makes contributing to this project a little harder.

Perhaps you can still contribute the compiler tests here and the other changes to compiler common and I'll make sure to push both changes into a branch which runs CI.

@alexdao3
Copy link
Contributor Author

alexdao3 commented Nov 8, 2022

@borkdude Thanks for the heads up, and no worries. I was a bit slow today to push up updates, so maybe you didn't expect there to be conflicts.

Anyways I cherry-picked the relevant commits for compiler-common and applied them in squint-cljs/compiler-common#3. I'm having a bit of trouble with local dev and wondering if you might have a tip. I have the following dir structure

code/cherry
code/squint/compiler-common

From /code/cherry, I run bb dev but am getting an error that particular imports from compiler-common doesn't exist. However, I see them in the squint/compiler-common code:

 File: /Users/alex/code/cherry/src/cherry/compiler.cljc:11:1
--------------------------------------------------------------------------------
   8 | ;; agreeing to be bound by the terms of this license.  You must not
   9 | ;; remove this notice, or any other, from this software.
  10 |
  11 | (ns cherry.compiler
-------^------------------------------------------------------------------------
Invalid :refer, var squint.compiler-common/*recur-targets* does not exist
--------------------------------------------------------------------------------
  12 |   (:require
  13 |    #?(:cljs [goog.string.format])
  14 |    #?(:clj [cherry.resource :as resource])
  15 |    [cherry.internal.deftype :as deftype]

Wonder if there's a caching issue somewhere in my local setup. I'll dig into it

(is (str/includes? s "local_file_some_fn.call(null)")))

(let [s (cherry/compile-string "(ns test-namespace (:require [clojure.core :as clojure-core])) (clojure-core/some-fn)")]
(is (str/includes? s "import { some_fn as clojure_core_some_fn } from 'clojure-core'"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Cherry didn't previously support requiring Clojure namespaces, I wasn't sure what the output here should be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support kebab-case namespace alias

2 participants