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

Use Yarn for dependency management and running tasks #3226

Closed
wants to merge 12 commits into from

Conversation

dtinth
Copy link

@dtinth dtinth commented Jan 11, 2019

This PR aims to modernize the build system a little bit, to make it easier for new contributors (me included) to hack on and contribute to this project. Goals:

  • To reduce the amount of manual steps required to set up a development environment.
  • …while keeping all the existing build commands (cake) intact.
  • To reduce the amount of global npm dependencies.
  • To have a better integration with VS Code.

What I did:

  1. Instead of requiring users to install coffeescript globally (npm install -g coffeescript) they only have to run yarn. This will:

    • Install the necessary dependencies (except PhantomJS) locally.
    • Run git submodule update to pull in shoulda.js (via postinstall script).
    • Run cake build (via prepare script).

    After this the project is immediately ready to be installed as a Chrome extension. It’s also ready to be tested (using yarn test) if the user have PhantomJS installed.

    The cake command is available as yarn cake. (yarn behaves somewhat like bundle exec here, allowing developers to run locally-installed CLI apps.)

  2. Since the project now follows npm script conventions the Travis CI configuration has been simplified.

  3. Relevant documentation has been updated (in CONTRIBUTING.md).

    • Steps has been reduced.
    • Make clear in the docs that PhantomJS v1.9.20 is required (the DOM tests fail in latest version of PhantomJS, but this is out of scope of this PR.)
  4. Added .vscode/tasks.json for better integration with VS Code. When opening the project in VS Code, pressing Cmd+Shift+B will launch the autobuild task.

    image

Related: #3029.

@smblott-github
Copy link
Collaborator

I agree that the current build system is showing its age. However, is this PR really the direction to go?

I'm on Debian, so -- on a fresh clone -- I'd like to just say...

npm install .
make build
make test
make autobuild

That would be pretty easy to do and would work on Mac too, but probably wouldn't be very Windows friendly.

@dtinth
Copy link
Author

dtinth commented Jan 11, 2019

I cannot say about whether this is "the" direction to go (as I am not a maintainer; just a user that wants to hack on this project and customize it for my own use), but I would say it is a step forward 😄. At the same time I try to make this PR fully backwards-compatible; it should have no impact for existing maintainers, but would improve the experience for new contributors.

This is the proposed workflow in this PR:

yarn install      # This also runs `git submodule update` and `cake build`
yarn test
yarn autobuild

There are 3 parts in this PR:

  1. package.json
  2. Yarn or npm (yarn.lock or package-lock.json)
  3. .vscode/tasks.json

Here are my opinions:

  1. package.json is definitely a way to go.

  2. Yarn vs npm: I don’t have a strong opinion here, but I chose Yarn for this PR because:

    • The yarn command lets you easily run locally installed packages. Instead of ./node_modules/.bin/cake, we can use yarn cake.

    • yarn is also preferred in VS Code’s codebase.

  3. .vscode/tasks.json

It would be great to hear some more second opinions. 😃

@smblott-github
Copy link
Collaborator

It would be great to hear some more second opinions.

Indeed.

@philc
Copy link
Owner

philc commented Jan 5, 2020

Thanks for proposing this. I'm keeping this in mind as we modernize the codebase.

Soon the coffeescript dependency and associated cake tasks will be gone. Then, the only other packages required will be whatever we need for running tests. This might make yarn less added value. I'd love to retire cake for a Makefile, but I'm not sure that will happen. There's a decent hunk of logic in the cakefile related to test running and code coverage.

@philc
Copy link
Owner

philc commented May 22, 2020

The cakefile is now gone, as is the transpilation and autobuilding step as of #3459, so we now have very few dependencies and no build steps. As a result, we don't need to add something as capable as yarn.

Note that I added a package.json as of 1f4f4e6, but I don't think we need this long term given how few dependencies we have, and I may remove it.

@philc philc closed this May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants