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

Write hash in pre-commit hook for predicable viewer.js location, build JS in CI #107

Merged
merged 44 commits into from
Mar 8, 2022

Conversation

borkdude
Copy link
Collaborator

@borkdude borkdude commented Mar 7, 2022

Fixes #102

@mk Ready for review.

@borkdude borkdude requested a review from mk March 7, 2022 20:16
Copy link
Member

@mk mk left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good and getting this should nicely improve the workflow. Only wondering if we can do a pass on the names here to make it clearer what this is for.

Some ideas:

  • write-hashwrite-viewer-resources-hash
  • hashing.cljviewer-resources-hashing
  • refresh-cdnbuild+upload-viewer-resources
  • front-end-hash.txtviewer-js-hash.txt, or viewer-resource-manifest if we turn it into the same map that clerk.resource_manifest is.

Alternatives to resources could be assets (which I know you don't like) or to stay as narrow as possible viewer-js. Though I do want to add building a css file here very soon.

Note that in clerk.resource_manifest we already use a custom css if that key is set.

Now thinking that we should unify those names, so either change the clerk.resource_manifest system property to become clerk.viewer_resource_manifest or drop the viewer from the suggestions above. Leaning towards with viewer, wdyt?

bb/hashing.clj Outdated Show resolved Hide resolved
bb.edn Outdated Show resolved Hide resolved
@borkdude
Copy link
Collaborator Author

borkdude commented Mar 8, 2022

@mk Excellent suggestions. I also renamed the bb dir to src-bb which is maybe a better name. I processed all suggestions. We can do more renaming in the future as necessary. I'll fix the build as renaming things almost always is paired with build failures :P.

@borkdude
Copy link
Collaborator Author

borkdude commented Mar 8, 2022

@mk Implemented the changes. One thing I'm wondering about. What if we get hash collisions, e.g.

gs://nextjournal-cas-eu/data/lookup/2MnmwsYfak9x5j6cXDBB56uRiheT

already exists, but was produced from a different set of files a year or so ago. Should we do additional cache invalidation?

@borkdude
Copy link
Collaborator Author

borkdude commented Mar 8, 2022

As discussed: collisions aren't considered a problem. Ready for squash/merge!

@borkdude borkdude merged commit e031d51 into main Mar 8, 2022
@borkdude borkdude deleted the compile-time-hash branch March 8, 2022 12:50
Copy link
Member

@mk mk left a comment

Choose a reason for hiding this comment

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

Some remarks which we might want to address in a follow-up.

bb/viewer_resources_hashing.clj Show resolved Hide resolved
resources/viewer-js-hash Show resolved Hide resolved
src/nextjournal/clerk/config.clj Show resolved Hide resolved
src/nextjournal/clerk/config.clj Show resolved Hide resolved
@borkdude
Copy link
Collaborator Author

borkdude commented Mar 8, 2022

Thanks, will resolve in follow up.

borkdude added a commit that referenced this pull request Mar 8, 2022
@borkdude
Copy link
Collaborator Author

borkdude commented Mar 8, 2022

Processed all comments and pushed to the ui tests branch (to avoid merge conflicts, rebasing, etc, I'd like to limit the work in progress).

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.

Include file in repo that contains CAS-hash to JS asset
2 participants