Skip to content
This repository was archived by the owner on May 31, 2020. It is now read-only.

Ready to Merge: Rationalise vendored (external) JS dependencies - #302

Merged
glasnt merged 1 commit into
beeware:masterfrom
candeira:browserify_vendored
Oct 14, 2016
Merged

Ready to Merge: Rationalise vendored (external) JS dependencies#302
glasnt merged 1 commit into
beeware:masterfrom
candeira:browserify_vendored

Conversation

@candeira

@candeira candeira commented Oct 14, 2016

Copy link
Copy Markdown
Contributor
  • external dependencies no longer artisanally bundled

  • vendoring workflow uses native-JS tools: node, yarn, browserify

  • VENDORING.md documents the workflow as if batavia developers
    are complete JS n00bs.

  • vendored_config.js added for those dependencies that have optional
    configuration steps.

    In addition, the workflow addresses the following constraints:

  • JS tools are only needed when adding a new dependency:
    once the vendored.js dependencies file has been built,
    batavia.js is still built using make and cat, exclusively
    from files existing in the batavia repository.

  • npm is only used to download code into the batavia tree,
    once per dependencies update/commit.

  • developers who are just consuming dependencies (not adding
    them) can remain oblivious of npm, node, yarn, etc.

  • the project can be built independently of npm access.

@candeira
candeira force-pushed the browserify_vendored branch from d3b0aca to b4218f8 Compare October 14, 2016 07:36
@candeira candeira changed the title Rationalize vendored (external) JS dependencies Rationalise vendored (external) JS dependencies Oct 14, 2016
@candeira

Copy link
Copy Markdown
Contributor Author

Note that there is currently a bug in yarn that breaks the vendoring script: yarnpkg/yarn#809. Fix is already merged, it will probably be on npm in a couple of days.

Anyone who wants to test the vendoring workflow can run the script with npm run vendor in the interim.

@freakboy3742

Copy link
Copy Markdown
Member

This certainly looks like a reasonable suggestion; but I won't profess any particular expertise on the way you're proposing to use the JS stack.

@swenson @glasnt Do you have any thoughts?

@glasnt glasnt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See inline comments

dependencies. Batavia developers consuming already vendored dependencies don't
need to know anything else or install extra tools.

The code for the vendored dependencies is bundled in the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this the rational behind committing node_modules?

Given batavia already depends on a make command, can we append base tooling in there in order to get the node_modules contents?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @glasnt, thanks for the quick replies.

The rationale behind committing node_modules to repo is that @freakboy3742 requested that coupling to external code sources be kept to a minimum, and I didn't argue, because there are good reasons for that.

Access to npm repos (or to github, should we decide to use source dependencies instead of packaged ones) should be only required for the action of downloading and committing the code for each dependency. Not for building the project.

Given a batavia git repo, anyone should be able to work without installing node & other JS tooling. Batavia prides itself in being a welcoming, accessible project, and complexity of tooling would be a limit to that, and not worth it in the absence of other benefits.

Also, with the proposed JS vendoring system, one can download the Batavia repo, go offline, switch branches around, and nothing that previously work will stop working because you went offline. If Make depended on npm/yarn to download JS dependencies on demand, you could git branch checkout <foo> and find yourself unable to work because crucial pieces are missing.

In the future, we will probably want to move the whole Batavia build over to a Node.js based tool, and gain the advantages of source maps, dead code elimination, etc. Giles Bowkett persuaded me of this with this article of his about letting the ruby on rails asset pipeline die. If we think of it, Batavia is not a Python project, but a JS one with some Python in it, same as CPython is primarily a C project with some Python in it. I know that I'm mostly writing JS, with a tiny bit of Python when I fiddle with tests.

If/when I learn how to work with JS build systems better and work the above source maps etc magic, I will propose a node-based build tool to replace npm. Then the benefits will outweigh the complexity disadvanages. However, even in that case I'd still advocate for keeping the node_modules directory versioned in-repo, for the stated reasons.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for this detailed response. With this reasoning, I completely agree.

Linking to this response is something I'll be doing when I find the right place for it in the doc guide

Comment thread batavia/vendor/yarn.lock
@@ -0,0 +1,21 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not 100% across what yarn brings to the table, but i'd appreciate if someone could enlighten me, or link to something that says why we should be using an extremely new, in-flux packaging system. If it gives super huge benefits over node, then fine. I just want to be informed.

@candeira candeira Oct 14, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

First, some nomenclature: npm is one name given to two things: the packaging backend (PyPi) and the client (pip). We're not using yarn over node: instead of node-npm(tool)-npm(repo) we're using node-yarn-npm(repo).

Yarn is a better npm. It has the advantage that it resolves dependencies deterministically, ie: always gets the same results for the same input file. The .lock file is the mechanism by which it does that, by not trusting the version number, but hashing the received file contents. Then you commit in the same commit the file that you edited (package.json) and the one that the tool wrote (yarn.lock). This keeps one from making the mistake that, because the input to the dependency management system looks the same, the output is also the same.

I've been working with other languages that have similar dependency management systems (like hex, the dependency/build tool for Elixir)., and I'm persuaded this reproducible-build way is the right way to do things.

That's why I decided to pick yarn. For our use, it's better, without much risk, since we could easily switch between npm(tool) and yarn. There is zero lock-in.

@glasnt

glasnt commented Oct 14, 2016

Copy link
Copy Markdown
Member

We probably want @phildini in on this one as well

@candeira
candeira force-pushed the browserify_vendored branch from b4218f8 to 9bff525 Compare October 14, 2016 09:28
  - external dependencies no longer artisanally bundled
  - vendoring uses native-JS tools: node, yarn, browserify
  - VENDORING.md documents the workflow as if batavia developers
    are complete JS n00bs.
  - library configuration spun into 'vendored_config.js'

  In addition, the workflow addresses the following constraints:

  - JS tools are only needed when adding a new dependency:
    once the vendored.js dependencies file has been built,
    batavia.js is still built using make and cat, exclusively
    from files existing in the batavia repository.
  - npm is only used to download code into the batavia tree,
    once per dependencies update/commit.
  - developers who are just consuming dependencies (not adding
    them) can remain oblivious of npm, node, yarn, etc.
  - the project can be built independently of npm access.
@candeira
candeira force-pushed the browserify_vendored branch from 9bff525 to c43212c Compare October 14, 2016 09:42
@candeira candeira changed the title Rationalise vendored (external) JS dependencies Ready to Merge: Rationalise vendored (external) JS dependencies Oct 14, 2016

@glasnt glasnt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With the rational, this is 👍 from me.

@glasnt
glasnt merged commit 021bf67 into beeware:master Oct 14, 2016
@candeira
candeira deleted the browserify_vendored branch October 18, 2016 06:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants