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

Migrate figwheel conf from project.clj to figwheel ns #3358

Merged

Conversation

voxlet
Copy link
Contributor

@voxlet voxlet commented Feb 18, 2018

Fixes #2999

Summary:

Replaced the figwheel profile :cljsbuild section with the equivalent in figwheel.edn format (along with the config reshaping code in figwheel-api), making sure not to change the behavior of figwheel-api/start.

Review notes (optional):

I erred on the conservative side with the changes, preserving the *-test builds and warning-handler in the android build.

Steps to test:

  • Confirm lein figwheel-repl behavior has not changed

status: ready

@status-github-bot
Copy link

Hey @voxlet, thanks for making your first pull request in status-react! ❤️

Copy link
Contributor

@jeluard jeluard left a comment

Choose a reason for hiding this comment

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

Thanks looks great!


(defn get-build [builds id]
(when-let [build (first (filter #(= (:id %) (get-id id)) builds))]
(if (test-id? id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this part still relevant? If not we could simply remove it.

Copy link
Contributor Author

@voxlet voxlet Feb 20, 2018

Choose a reason for hiding this comment

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

Not that I can see, at least in terms of immediate code dependency. It was kept for the case of everyday REPL session use, but as far as I can tell the *-test runs are already broken in develop.

Maybe we can go ahead and delete the code, and see if there any complaints? I think the chances of that happening are low enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove it then! :)

@jeluard jeluard requested a review from dmitryn February 19, 2018 07:48
Copy link
Contributor

@dmitryn dmitryn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@voxlet
Copy link
Contributor Author

voxlet commented Feb 20, 2018

@jeluard Thanks for the feedback, removed the test builds.
Also migrated the key names to the external figwheel.edn format (described here), and moved the static nREPL port config there as well.

@flexsurfer
Copy link
Member

@voxlet please squash commits

@voxlet voxlet force-pushed the feature/simplify-figwheel-config-#2999 branch from 434ddd0 to fd3cc86 Compare February 20, 2018 20:45
Signed-off-by: Julien Eluard <julien.eluard@gmail.com>
@jeluard jeluard force-pushed the feature/simplify-figwheel-config-#2999 branch from fd3cc86 to fad4750 Compare February 21, 2018 07:48
@jeluard jeluard merged commit fad4750 into status-im:develop Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Simplify our figwheel config
4 participants