Use direct inclusion, not submodules, to bring Javelin into Phabricator
ClosedPublic
Actions

Authored by epriestley on Jan 22 2013, 6:56 PM.

Details

Reviewers

vrana
btrahan

Maniphest Tasks

Restricted Maniphest Task

Commits

Restricted Diffusion Commit
rP07767fda00ab: Use direct inclusion, not submodules, to bring Javelin into Phabricator
rPcc92749f52c8: Use direct inclusion, not submodules, to bring Javelin into Phabricator
rP85554c1f2d0e: Use direct inclusion, not submodules, to bring Javelin into Phabricator

Summary

Submoduling is slightly convenient for developers but hellishly difficult for many users. Since we make about a dozen updates to Javelin per year, just include the source directly.

Even if we run git submodule status more often, this creates additional problems for users with PATH misconfigured.

Fixes T2062 by nuking it from orbit.

Test Plan

Loaded site, browsed around. Grepped for references to submodules.

Diff Detail

Branch

nosub

Lint

Lint Errors

Severity	Location	Code	Message
Error	externals/javelin/src/ext/view/HTMLView.js:0	JAVELIN3	Unnecessary Javelin Dependency
Error	externals/javelin/src/lib/Resource.js:0	JAVELIN3	Unnecessary Javelin Dependency
Error	externals/javelin/src/lib/Resource.js:0	JAVELIN3	Unnecessary Javelin Dependency
Error	externals/javelin/src/lib/control/typeahead/source/TypeaheadOnDemandSource.js:0	JAVELIN3	Unnecessary Javelin Dependency
Error	externals/javelin/src/lib/control/typeahead/source/TypeaheadPreloadedSource.js:0	JAVELIN3	Unnecessary Javelin Dependency

Unit

Tests Passed

Event Timeline

Fixes T2062 by nuking it from orbit.

haha!

Some of the lint warnings seem legit to me re: missing or unnecessary dependencies.

I wish Git submodules work more automatically.

Why is this diff requiring javelinsymbols in path?

Previously, we required javelinsymbols be built in externals/javelin/, but I'm including only javelin/src/ explicitly here, not all of the support/documentation/website/examples/prebuilt packages, so the source to build javelinsymbols is no longer in the project.

We could include all the code required to build javelinsymbols, but it seems like overkill since only developers who are touching JS have any need for it.

I don't like putting everything in the path (especially the tools I don't run manually) so I would prefer specifying the path manually but I don't care much about it.

That makes sense. We should document how to build it anyway; I'll file something to make this better and more configurable.

Closed by commit rP85554c1f2d0e (authored by @epriestley).

I backed this out and reverted "master" on the origin to before the commit. When I landed this as is, "git pull" began to fail in working copies with the submodule checked out:

$ git pull --rebase
First, rewinding head to replay your work on top of it...
error: The following untracked working tree files would be overwritten by checkout:
	externals/javelin/.gitignore
	externals/javelin/LICENSE
	externals/javelin/README
	externals/javelin/src/core/Event.js
  ...

I'm going to move externals/javelin/ to some other location to prevent this from happening.

chad mentioned this in T2378: Document how to build `javelinsymbols` and allow its location to be specified rather than read from $PATH.Feb 24 2017, 9:47 PM

epriestley changed the visibility from "All Users" to "Public (No Login Required)".Nov 2 2018, 5:13 PM