Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ClojureScript support #524

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

Add ClojureScript support #524

wants to merge 36 commits into from

Conversation

furkan3ayraktar
Copy link
Collaborator

@furkan3ayraktar furkan3ayraktar commented Dec 26, 2024

This PR adds ClojureScript support to the poly tool. There is a longer discussion explaining the rationale behind here.

Some of the decisions to keep the scope of the changes small without compromising the overall value:

  • All components, bases, and projects live under a single Polylith monorepo regardless of the dialects used within them.
  • The components and bases can mix clj, cljs, and cljc files in their source files.
  • The workspace.edn will have a new config option called :dialects to let users restrict dialects in their workspace.
  • The poly tool will also read cljs files, in addition to the cljc and clj files, if cljs is enabled in the current workspace.
  • A component or base with a modified file will be marked as changed regardless of the dialect. As a result, all dependent components will be marked as indirectly changed.
  • We decided to leave testing out of scope and rely on shadow-cljs or similar for testing.
  • The Polylith team will provide a simple development setup for a mixed (or ClojureScript only) workspace using shadow-cljs. The users can choose their own way to create a classpath and start their own clj/cljs REPLs.

TODOs:

  • Add dialects to workspace config
    • The :dialects config option works as follows. The user can define a set of dialects. Valid dialects are: "clj" and "cljs". If the dialects option is not defined or if it's an empty set, it will be treated as if it is #{"clj"}. This is necessary for backward compatibility. If it is #{"clj", "cljs}, .clj, .cljs, and .cljc files will be considered as valid code sources.
  • Handle require-macros case
    • ClojureScript files can require macros from clj files using :require-macros.
  • Ensure the default test runner works without breaking
    • The external test runner will need an update for ClojureScript-enabled workspaces.
  • Handle NPM dependencies in the libs command
    • Read the package.json files and include those dependencies in the libs command.
    • Each brick and project should have a package.json if they use NPM dependencies.
  • Update the create command to support creating ClojureScript bricks and projects.
  • Add an example project under examples with corner cases.
    • Project should contain:
      • A component with only clj sources
      • A component with clj and cljc sources
      • A component with clj, cljs, and cljc sources
      • A component with only cljs sources
      • A component with a cljs source and an accompanying clj source with macro definitions. The cljs file should require a clj file using require-macros. There should be additional clj files that the main clj file that contains the macros should require regularly.
  • Create a sample development environment & projects with shadow-cljs
  • Update the documentation
    • Explain :dialects key
    • Explain how to work with ClojureScript (new section)
  • Update the FAQ for ClojureScript support (gitbook)

@furkan3ayraktar furkan3ayraktar added core Related to core functionality of Polylith cli Related to Polylith cli labels Dec 26, 2024
@seancorfield
Copy link
Contributor

When you get around to this:

Update the create command to support creating ClojureScript bricks and projects.

I recommend adding new templates under creator/templates/<dialect>/ but otherwise matching the structure of the default templates.

The search order can then be:

  • per-project, per-dialect template, else fall back to
  • per-user, per-dialect template, else fall back to
  • default (internal), per-dialect template, else fall back to
  • per-user template, else fall back to
  • per-project template, else fall back to
  • default (internal) template (else error)

It would be simpler to do the per-dialect to default fall back on a per-tier basis but that could cause a user's (Clojure) template to take precedence over the default ClojureScript one, which is not what we want.

This ensures the new template machinery is purely additive and will not break anyone's existing templates, and it would also allow for templates/clj/ to have Clojure-specific be different from the default if users want that.

@seancorfield
Copy link
Contributor

An additional thought: when the dialect is plumbed into the creator component, it would be a good idea to add :dialect dialect to the data passed into the template renderer, so that templates can contain conditional logic to produce different output for clj/cljs if users want:

{% ifequal dialect "cljs" %}
... some cljs-specific code ...
{% else %}
... some clj-specific code ...
{% endifequal %}

@seancorfield
Copy link
Contributor

@tengstrand I added a couple of notes to your latest commit: there are a couple more places that need dialect passed into templates.

The new templates.adoc file will also need updating to note that {{dialect}} is a new variable in all the templates.

@@ -107,8 +110,10 @@ This command uses the following template files to generate the new base:
* `templates/bases/deps.edn` -> the new base's `deps.edn` file
* no variable substitutions are provided
Copy link
Contributor

Choose a reason for hiding this comment

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

All the deps.edn files that currently say no variables substitutions are provided need to be updated to say: {{dialect}} -- the dialect, e.g. "cljs" (from the create component command)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please have a look again and check if I got it right this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like templates/components/interface.clj is missing the {{dialect}} variable in the doc page.

I intended the list of variables to be indented bullets under each filename bullet but AsciiDoc didn't treat * (<space>*) the same as Markdown does -- should those variable lines all be ** bullets? Not sure how AsciiDoc handles that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're absolutely right — I hadn't noticed that! I've now updated the sub-bullets to use ** as intended and also fixed the other issue.

furkan3ayraktar and others added 20 commits January 12, 2025 07:26
When we read the source files, we are interested in the require and import statements in the namespace declaration. We use these statements to determine which interface and library dependencies each component has. When there are reader conditionals within these statements, and the workspace is configured to use both `clj` and `cljs` dialects, we need to include imports and requires for both `clj` and `cljs`.

We read the source files using edamame. We can pass an option (`:features`) to edamame to choose between reader conditionals when reading the source files. However, when passing both `:clj` and `:cljs` to edamame, it takes the first match from the conditional instead of both. Luckily, it has another option, `:read-cond`, and it accepts a function that receives the complete conditional statement, and we can decide what to return in place of the conditional.

I implemented a function to read both `clj` and `cljs` parts of the reader conditional when both `clj` and `cljs` dialects are enabled for the workspace. Otherwise, I use `:read-cond :allow` to rely on edamame's default implementation.
Comment on lines +21 to +25
(->> test-brick-names
(mapcat brick-name->namespaces)
(filter clj-namespace?)
(map :namespace)
(into []))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(->> test-brick-names
(mapcat brick-name->namespaces)
(filter clj-namespace?)
(map :namespace)
(into []))))
(into []
(comp (mapcat brick-name->namespaces)
(filter clj-namespace?)
(map :namespace))
test-brick-names)))

Comment on lines +29 to +32
(->> (:test namespaces)
(filter clj-namespace?)
(map :namespace)
(into []))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(->> (:test namespaces)
(filter clj-namespace?)
(map :namespace)
(into []))))
(into []
(comp (filter clj-namespace?)
(map :namespace))
(:test namespaces))))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to Polylith cli core Related to core functionality of Polylith
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants