Skip to content

Commit

Permalink
Clean up some meteor#3006 comments
Browse files Browse the repository at this point in the history
  • Loading branch information
glasser committed Dec 2, 2014
1 parent 06fbe35 commit ab3d6c5
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 15 deletions.
9 changes: 5 additions & 4 deletions tools/commands-packages.js
Original file line number Diff line number Diff line change
Expand Up @@ -1670,9 +1670,11 @@ var maybeUpdateRelease = function (options) {
var upgradersToRun = upgraders.upgradersToRun(projectContext);

// Download and build packages and write the new versions to .meteor/versions.
// XXX #3006 If we're about to try to upgrade packages, do we really want to
// download and build packages here? Note that if we change this,
// that changes the upgraders interface.
// XXX It's a little weird that we do a full preparation for build
// (downloading packages, building packages, etc) when we might be about
// to upgrade packages and have to do it again. Maybe we shouldn't? Note
// that if we change this, that changes the upgraders interface, which
// expects a projectContext that is fully prepared for build.
main.captureAndExit("=> Errors while initializing project:", function () {
projectContext.prepareProjectForBuild();
});
Expand Down Expand Up @@ -2247,7 +2249,6 @@ main.registerCommand({
// admin make-bootstrap-tarballs
///////////////////////////////////////////////////////////////////////////////

// XXX #3006 make sure this still works
main.registerCommand({
name: 'admin make-bootstrap-tarballs',
minArgs: 2,
Expand Down
1 change: 0 additions & 1 deletion tools/package-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,6 @@ _.extend(PackageSource.prototype, {
// which needs to be loaded by the linker).
// XXX add a better API for js-analyze to declare itself here
if (self.name !== "meteor" && self.name !== "js-analyze" &&
// XXX #3006 is this still needed?
!process.env.NO_METEOR_PACKAGE) {
// Don't add the dependency if one already exists. This allows the
// package to create an unordered dependency and override the one that
Expand Down
4 changes: 4 additions & 0 deletions tools/project-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ _.extend(exports.ProjectContext.prototype, {
self.packageMapDelta = null;

// Initialized by _buildLocalPackages.
// XXX #SoftRefresh Perhaps save the old IsopackCache and try
// to reuse unchanged in-memory Isopack objects?
// Be careful to only save one old IsopackCache, not a
// linked list of them. #3213
self.isopackCache = null;

self._completedStage = STAGE.INITIAL;
Expand Down
19 changes: 11 additions & 8 deletions tools/run-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -404,11 +404,14 @@ _.extend(AppRunner.prototype, {
if (! firstRun) {
// If this isn't the first time we've run, we need to reset the project
// context since everything we have cached may have changed.
// XXX #3006 We can try to be a little less conservative here:
// XXX We can try to be a little less conservative here:
// - Keep around some in-memory Isopack objects and validate them
// by their buildinfo (we used to call this Soft Refresh).
// - Don't re-build the whole local catalog if we know which ones
// have changed.
// by their buildinfo (we used to call this #SoftRefresh).
// - Don't re-build the whole local catalog if we know which local
// packages have changed. (This one might be a little trickier due
// to how the WatchSets are laid out. Might be possible to avoid
// re-building the local catalog at all if packages didn't change
// at all, though.)
self.projectContext.reset();
var messages = buildmessage.capture(function () {
self.projectContext.readProjectMetadata();
Expand Down Expand Up @@ -511,8 +514,8 @@ _.extend(AppRunner.prototype, {
outcome: 'outdated-cordova-platforms'
};
}
// XXX #3006 This is racy --- we should get this from the pre-runner build,
// not from the first runner build.
// XXX This is racy --- we should get this from the pre-runner build, not
// from the first runner build.
self.cordovaPlatforms = platforms;

var plugins = cordova.getCordovaDependenciesFromStar(
Expand All @@ -523,8 +526,8 @@ _.extend(AppRunner.prototype, {
outcome: 'outdated-cordova-plugins'
};
}
// XXX #3006 This is racy --- we should get this from the pre-runner build,
// not from the first runner build.
// XXX This is racy --- we should get this from the pre-runner build, not
// from the first runner build.
self.cordovaPlugins = plugins;

var serverWatchSet = bundleResult.serverWatchSet;
Expand Down
2 changes: 1 addition & 1 deletion tools/tests/report-stats.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ var clientAddress;
// NOTE: This test will fail if your machine's time is skewed by more
// than 30 minutes. This is because the `fetchAppPackageUsage` method
// works by passing an hour time range.
// XXX #3006 Have not managed to get this test passing since introducing
// XXX I have not managed to get this test passing since introducing
// isopack-cache, though it seems to just be major server slowness
// and perhaps preexisting.
selftest.define("report-stats", ["slow", "net"], function () {
Expand Down
1 change: 0 additions & 1 deletion tools/updater.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ var updateMeteorToolSymlink = function () {
// symlink points to. Let's make sure we have that release on disk,
// and then update the symlink.
try {
// XXX #3006 how does this work with respect to progress bars?
buildmessage.enterJob({
title: "Downloading tool package " + latestRelease.tool
}, function () {
Expand Down

0 comments on commit ab3d6c5

Please sign in to comment.