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.

Sign up for GitHub

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

Jump to bottom

feat(cli/templates) add clojurescript #185

Merged
merged 8 commits into from
Sep 22, 2022
Merged

feat(cli/templates) add clojurescript #185

merged 8 commits into from
Sep 22, 2022

Conversation

just-sultanov
Copy link
Contributor

@just-sultanov just-sultanov commented Sep 19, 2022 •
edited
Loading

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

Related to this nuxt/framework#184

@just-sultanov just-sultanov force-pushed the feature-clojurescript-template branch from d9f392a to 68f3044 Compare September 20, 2022 06:19
amrbashir
amrbashir requested changes Sep 20, 2022
View reviewed changes
Copy link
Member

@amrbashir amrbashir left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You also need to add a post init note about installing java and clojure
A similar thing has been done for yew template in packages/cli/src/templates.rs in post_init_info method

Also don't forget to run carg fmt --all and pnpm format in the repo root

Thanks.

packages/cli/fragments/fragment-clojurescript/_cta_manifest_ Outdated Show resolved Hide resolved
packages/cli/fragments/_assets_/cljs.svg Outdated Show resolved Hide resolved
packages/cli/fragments/fragment-clojurescript/shadow-cljs.edn Outdated Show resolved Hide resolved
packages/cli/fragments/fragment-clojurescript/_cta_manifest_ Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
.github/workflows/templates-test.yml Outdated Show resolved Hide resolved
packages/cli/fragments/fragment-clojurescript/src/main/clojure/app/core.cljs Outdated Show resolved Hide resolved
@just-sultanov
feat(cli/templates): add clojurescript
0bef5a5
@just-sultanov
fix(cli/fragments/clojurescript): code review improvements
409ac3e 
- add a post init note about installing `java` and `clojure`
- add an example using the `greet` command
- move some resources to the `clojurescript` template
- change ports for `shadow-cljs` and `tauri`
- update ci pipeline
- fix code formatting
@just-sultanov just-sultanov force-pushed the feature-clojurescript-template branch from 9623778 to 409ac3e Compare September 20, 2022 19:44
amrbashir
amrbashir requested changes Sep 20, 2022
View reviewed changes
Copy link
Member

@amrbashir amrbashir left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I should've caught this in my review but the template shouldn't include testing so please remove all test related files/deps/scripts...etc

packages/cli/fragments/fragment-clojurescript/src/test/clojure/app/core_test.cljs Outdated Show resolved Hide resolved
packages/cli/fragments/fragment-clojurescript/package.json Outdated Show resolved Hide resolved
packages/cli/fragments/fragment-clojurescript/package.json Outdated Show resolved Hide resolved
packages/cli/fragments/fragment-clojurescript/karma.config.js Outdated Show resolved Hide resolved
@amrbashir
Copy link
Member

CI seems to be failing to build the clojure project

@just-sultanov
fix(cli/fragments/clojurescript): code review improvements
da461bd 
- added the recommended `calva` plugin for `vscode`
- updated deps
- removed exclusions for the `reagent` dependency
@just-sultanov
Copy link
Contributor Author

just-sultanov commented Sep 22, 2022 •
edited
Loading

CI seems to be failing to build the clojure project

I don't know why deps.edn can't be found.
OK, I'll switch to using only shadow-cljs without deps.edn. Local testing of commands on mac/ubuntu passes without any problems.

@just-sultanov just-sultanov force-pushed the feature-clojurescript-template branch from 8d394e8 to 341d3a6 Compare September 22, 2022 13:00
@just-sultanov
Copy link
Contributor Author

@amrbashir all tests were passed. Only pnpm doesn't work.

Cause:
The required JS dependency "object-assign" is not available, it has been requested "node_modules/react/cjs/react.production.min.js".

I can fix it using the following methods:

  1. Add .npmrc to the root of the clojurescript template to allow install dependencies using pnpm i --shamefully-hoist
  2. Add object-assign dependency to package.json
  3. Support only yarn and npm package managers

What do you think about it?

@nothingismagick
Copy link
Member

In my opinion #2 seems the best, because #3 is the worst.

I read through similar troubles over at nuxt (maybe there is another solution?
nuxt/nuxt#14146

@amrbashir
Copy link
Member

yeah option no.2 seems to be the best for now

@just-sultanov
Copy link
Contributor Author

Unfortunately, option no.2 didn't work. I added a .npmrc file with the required parameters for the specified dependencies (without wildcards). In addition, I have added comments describing the problem with the compiler.

@amrbashir
Copy link
Member

amrbashir commented Sep 22, 2022 •
edited
Loading

That's fine, you can also create this file only if pnpm is the package manager so the file name would be like this _[pnpm]_.npmrc

@just-sultanov
fix(cli/fragments/clojurescript): fix issue with the pnpm
18eabdc 
These settings allow us to use `pnpm` as a package manager.
Without these settings, we will receive several errors from the compiler:
`object-assign`, `scheduler`, `scheduler/tracing` is not available.
@just-sultanov just-sultanov force-pushed the feature-clojurescript-template branch from 58de670 to 18eabdc Compare September 22, 2022 18:43
@just-sultanov
Copy link
Contributor Author

That's fine, you can also create this file only if pnpm is the package manager so the file name would be like this _[pnpm]_.npmrc

done

amrbashir
amrbashir previously approved these changes Sep 22, 2022
View reviewed changes
@amrbashir
Copy link
Member

Thank you

@amrbashir amrbashir merged commit 6ca747e into tauri-apps:dev Sep 22, 2022
@just-sultanov just-sultanov deleted the feature-clojurescript-template branch September 22, 2022 20:03
@just-sultanov just-sultanov mentioned this pull request Sep 22, 2022
Closed
@phrohdoh
Copy link

phrohdoh commented Mar 3, 2023

It appears as if these changes were lost sometime between them being merged and the moment I'm writing this comment as the .cljs files, the Template::ClojureScript match arm, and the templated files are not on dev (bca3828).

Any idea what happened here?

@FabianLars
Copy link
Member

Many templates were removed in #284
https://tauri.app/blog/2023/03/01/create-tauri-app-version-3-released#removing-templates

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

@amrbashir amrbashir amrbashir approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@just-sultanov @amrbashir @nothingismagick @phrohdoh @FabianLars