ClojureScript

Add support for renamePrefix and renamePrefixNamespace closure compiler options

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: 1.9.908
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Approval:
    Accepted

Description

Adding support for these closure compiler options would allow for a mechanism to avoid name collisions

  1. CLJS-2338.patch
    30/Aug/17 9:01 AM
    2 kB
    Pieter du Toit
  2. CLJS-2338.patch
    29/Aug/17 6:01 AM
    1 kB
    Pieter du Toit

Activity

Hide
Pieter du Toit added a comment - - edited

Patch attached

Show
Pieter du Toit added a comment - - edited Patch attached
Hide
David Nolen added a comment -

Pieter, thanks for the patch. Have you submitted your contributor agreement?

Show
David Nolen added a comment - Pieter, thanks for the patch. Have you submitted your contributor agreement?
Hide
David Nolen added a comment -

One question I have about the patch, is there a default prefix name if not supplied by the user?

Show
David Nolen added a comment - One question I have about the patch, is there a default prefix name if not supplied by the user?
Hide
Pieter du Toit added a comment -

Yes, I have submitted my contributor agreement. The patch does not specify defaults, if the user does not supply :rename-prefix or :rename-prefix-namespace, then the compiler options passed to the closure compiler would be unchanged.

Show
Pieter du Toit added a comment - Yes, I have submitted my contributor agreement. The patch does not specify defaults, if the user does not supply :rename-prefix or :rename-prefix-namespace, then the compiler options passed to the closure compiler would be unchanged.
Hide
David Nolen added a comment -

Pieter sorry for being unclear, what happens if :rename-prefix-namespace is specified but :rename-prefix is not specified?

Show
David Nolen added a comment - Pieter sorry for being unclear, what happens if :rename-prefix-namespace is specified but :rename-prefix is not specified?
Hide
Pieter du Toit added a comment -

AFAICT, the options can be specified independently, the only documentation I could find on it is in Closure CommandLineRunner and here.

For my own use case, I only need to set :rename-prefix, which I have tested both with and without :modules and works as expected.

I did some further testing now with only specifying :rename-prefix-namespace (which calls setRenamePrefixNamespace), it has no impact on a build without :modules, and results in an error "{prefix} is not defined" when using with :modules. Specifying both :rename-prefix and :rename-prefix-namespace for a build with :modules results in the same "{prefix} is not defined" error.

I am less confident in :rename-prefix-namespace so I could modify the patch to remove the option ? I also noticed that I would need to modify the patch to add any new options to known-opts

Show
Pieter du Toit added a comment - AFAICT, the options can be specified independently, the only documentation I could find on it is in Closure CommandLineRunner and here. For my own use case, I only need to set :rename-prefix, which I have tested both with and without :modules and works as expected. I did some further testing now with only specifying :rename-prefix-namespace (which calls setRenamePrefixNamespace), it has no impact on a build without :modules, and results in an error "{prefix} is not defined" when using with :modules. Specifying both :rename-prefix and :rename-prefix-namespace for a build with :modules results in the same "{prefix} is not defined" error. I am less confident in :rename-prefix-namespace so I could modify the patch to remove the option ? I also noticed that I would need to modify the patch to add any new options to known-opts
Hide
Thomas Heller added a comment -

FWIW I'm using setRenamePrefixNamespace for a feature in shadow-cljs [1]. The effect is that only one global variable will be created and everything else will be a property of that variable. I'm using $CLJS as the prefix namespace and where :advanced mode would otherwise create a xY variable (just an example, name doesn't matter), it would now create a $CLJS.xY property instead.

I have never used :rename-prefix but my understanding is that if would just turn xY into $CLJSxY so you'd still end up with a bunch of global variables.

In my case I use it combined with :modules and only setRenamePrefixNamespace needs to be set and is otherwise unrelated to :rename-prefix.

[1] https://github.com/thheller/shadow-cljs/blob/21e9a3a279ed7a34239cc3c7cc65715f22289163/src/main/shadow/cljs/closure.clj#L481

Show
Thomas Heller added a comment - FWIW I'm using setRenamePrefixNamespace for a feature in shadow-cljs [1]. The effect is that only one global variable will be created and everything else will be a property of that variable. I'm using $CLJS as the prefix namespace and where :advanced mode would otherwise create a xY variable (just an example, name doesn't matter), it would now create a $CLJS.xY property instead. I have never used :rename-prefix but my understanding is that if would just turn xY into $CLJSxY so you'd still end up with a bunch of global variables. In my case I use it combined with :modules and only setRenamePrefixNamespace needs to be set and is otherwise unrelated to :rename-prefix. [1] https://github.com/thheller/shadow-cljs/blob/21e9a3a279ed7a34239cc3c7cc65715f22289163/src/main/shadow/cljs/closure.clj#L481
Hide
David Nolen added a comment -

Ok so they sound like independent options then. Pieter, I'm fine with the patch granted it's updated to augment known-opts.

Show
David Nolen added a comment - Ok so they sound like independent options then. Pieter, I'm fine with the patch granted it's updated to augment known-opts.
Hide
Pieter du Toit added a comment -

David, thanks, I have attached the updated patch

Show
Pieter du Toit added a comment - David, thanks, I have attached the updated patch
Hide
Pieter du Toit added a comment -

Thomas, thanks, I get the same result (no prefix namespace) with a shadow-cljs release build as with cljs (when using the new :rename-prefix-namespace and no :modules)

shadow-cljs.edn
{:dependencies
 []

 :source-paths
 ["src"]

 :builds
 {:app
  {:target :browser
   :output-dir "shadow/js"
   :modules
   {:main {:entries [moduleb]}}}}}
moduleb.cljs
(ns moduleb)

(def env "dev")

(defn init []
  (js/console.log (str "Module B init for env: " env)))

(init)
Show
Pieter du Toit added a comment - Thomas, thanks, I get the same result (no prefix namespace) with a shadow-cljs release build as with cljs (when using the new :rename-prefix-namespace and no :modules)
shadow-cljs.edn
{:dependencies
 []

 :source-paths
 ["src"]

 :builds
 {:app
  {:target :browser
   :output-dir "shadow/js"
   :modules
   {:main {:entries [moduleb]}}}}}
moduleb.cljs
(ns moduleb)

(def env "dev")

(defn init []
  (js/console.log (str "Module B init for env: " env)))

(init)
Hide
Thomas Heller added a comment -

The option itself is not supported as of now as I'm using the default cljs.closure/set-options function in shadow-cljs. When it is added to CLJS it will become available in shadow-cljs.

I'm currently using the feature internally for the :npm-module target which compiles code so it can be directly consumed by JS tools (or node). I'm setting that manually though so the :rename-prefix-namespace option is not recognized.

Show
Thomas Heller added a comment - The option itself is not supported as of now as I'm using the default cljs.closure/set-options function in shadow-cljs. When it is added to CLJS it will become available in shadow-cljs. I'm currently using the feature internally for the :npm-module target which compiles code so it can be directly consumed by JS tools (or node). I'm setting that manually though so the :rename-prefix-namespace option is not recognized.

People

Vote (1)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: