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

Support versioning of ramda-repl js file #190

Merged
merged 1 commit into from
Oct 4, 2017

Conversation

zehua
Copy link
Contributor

@zehua zehua commented Sep 25, 2017

We started to use rawgit since #136. From the comment in ramda/repl#50 (comment), rawgit faq does not recommend using master as the tag as they do not clear cache.

This PR attempts to use tag from ramda-repl instead of always using master to overcome this issue.

I also added a util script to generate the repl/index.html from handlerbars. Sorry that I could not figure out how to use the new pug based template introduced in #175. It also looks like the current repl/index.html has been manually modified in the past few commits. Feel free to update the pug template to support the same, if this PR is accepted. @MattMS

TODO:

@craigdallimore @buzzdecafe

@MattMS
Copy link
Member

MattMS commented Sep 25, 2017

Thanks for the research on the rawgit versioning, I'm happy with the idea of adding the version number.

I don't really like reverting back to Handlebars, because then we lose the inheritance for common content of the site.

I'm also not a fan of including an unused dependency just to get the version number.
The simplest fix would be replacing master with 1.1.0 in repl/index.pug.
Otherwise you could add the repl_tag to make_repl_index_html.js and replace master with #{repl_tag}.
I prefer the former.

@zehua
Copy link
Contributor Author

zehua commented Sep 25, 2017

I don't really like reverting back to Handlebars, because then we lose the inheritance for common content of the site.

I did give the pug approach a try. But there's some styling issue with the width of the repl.

Alt text

In addition, it looks like the current production site is still using the old html and styles. I thought it's safer to just update based on the older version and wait for someone more familiar with the pug approach to proper test it and release it.

I'm also not a fan of including an unused dependency just to get the version number.
The simplest fix would be replacing master with 1.1.0 in repl/index.pug.
Otherwise you could add the repl_tag to make_repl_index_html.js and replace master with #{repl_tag}.
I prefer the former.

In this sense, we are also including ramda itself as an unused dependency, since we are really only using its version. I like explicit dependency. But this is a minor issue. I am ok with including 1.1.0 directly in the pug template.

In short, do you prefer me to directly update the repl/index.html with the new 1.1.0 tag in this PR and you taking another look at fixing the pug and less styling issue and launch the PUG approach properly in a separate PR? @MattMS I could take a stab at fixing the styling issue (it's probably this change). But it's up to the site maintainer to decide when to release this to production site.

@zehua
Copy link
Contributor Author

zehua commented Sep 25, 2017

I've updated the PR to just use the 1.1.0 value directly in the html.

I updated both repl/index.html for immediate release now, and also repl/index.pug for future release of the PUG based version.

The styling issue with PUG repl is addressed in a separate PR: #191

Let me know whether this works for you @MattMS

@zehua
Copy link
Contributor Author

zehua commented Oct 2, 2017

ping reviewers

@MattMS
Copy link
Member

MattMS commented Oct 4, 2017

Thanks @zehua, this all looks fine to me.

I'll check out your other PRs next.

@MattMS MattMS merged commit 7a89204 into ramda:master Oct 4, 2017
@MattMS
Copy link
Member

MattMS commented Oct 4, 2017

REPL with that version was not deployed.
I've reverted this until ramda/repl#51 lands.

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.

None yet

2 participants