Ticket #627 (closed defect: fixed)

Opened 3 years ago

Last modified 2 years ago

Bootstrap refactoring to reduce bloat and separate package loading

Reported by: jburke Owned by: jburke
Priority: normal Milestone:
Component: Core Version: 0.3
Severity: normal Keywords: bootstrap package load
Cc:

Description

This is a ticket to track the work described in this URL:

http://dojo.jot.com/WikiHome/BootstrapBloat

Attachments

bootstrapRefactor.patch (76.3 kB) - added by jburke 3 years ago.
Patch to trunk to refactor bootstrap files.
bootstrapPatch2.txt (76.6 kB) - added by jburke 3 years ago.
Pass #2 at patch to trunk to refactor bootstrap files.
bootstrapPatch2.tar (10.0 kB) - added by jburke 3 years ago.
Pass #2 at patch to trunk to refactor bootstrap files (New files)

Change History

Changed 3 years ago by jburke

I created a patch for this work. If it looks good, then I will apply it to the trunk. I created the patch by doing a svn diff against my version of trunk (r3562).

If possible, I would like to have Paul Sowden review the patch, but I welcome feedback from anyone on it. Some notes about the patch vs. the work that was described on the wiki page:

* bootstrap2.js now loads before the hostenv_*.js file.

* Put the addOnLoad stuff into bootstrap2.js, and the methods that back require and provide. This means for a dojo-lite that it would have to provide stub functions for this functionality.

* Kept dojo.hostenv.conditionalLoadModule and removed dojo.kwCompoundRequire. There were lots of references to conditionalLoadModule in package.js files, and these files also use dojo.hostenv.moduleLoaded(). It seemed odd to have dojo.kwCompoundRequire paired with dojo.hostenv.moduleLoaded().

* docscripts/parser.php: has a reference to dj_deprecated. Also, it is looking for dojo.hostenv.kwCompoundRequire, but it should just be dojo.kwCompoundRequire (but I removed that method now anyway).

* Removed requireAfterIf and used requireIf instead. However, I kept the requireAfterIf function defintion since it was used in a bunch of widgets. Didn't want to remove it yet, since I figured someone would holler (if they had done a copy/paste from the old code).

* Kept dojo.loaded() for now.

Stuff I didn't do, but might be worth considering.

* hostenv_adobesvg.js: affected by changes, but I don't think adversely.

* hostenv_browser.js: 1st part parses the query string to look for djConfig parameters. Is this used by anyone?

* hostenv_browser.js: 3rd part does browser detection and sets variables off of dojo.render. There is one block that is concerned with detecting Adobe SVG. Can it be removed? Look for the comment: // check for ASVG

* Didn't try to convert dojo.exists to dojo.evalObjPath. Anyone else want to give it a shot to see if they are interchangeable (just got lazy).

* bootstrap2.js: addedToLoadingCount: is modified by never read. Remove?

* bootstrap2.js: removedFromLoadingCount is modified by never read. Remove?

* dojo.hostenv.setModulePrefix and dojo.setModulePrefix. It would be nice to just go to dojo.setModulePrefix, but the hostenv version is mentioned extensively in docs.

* Lots of test files push into modulesLoadedListeners directly instead of using dojo.addOnLoad. It would be nice to change everyone to use addOnLoad.

I'll upload the patch to this ticket.

Changed 3 years ago by jburke

Patch to trunk to refactor bootstrap files.

Changed 3 years ago by psowden

  • _addHostEnv should be wrapped up in a closure so as not to remain in global scope.
  • dojo.debug should be added back in somewhere. I liked the idea of a debug.js, but I would be tempted to keep this seperate from browser_debug.js, so creating a new file to contain the lost debug methods seems like an idea.
  • Stub functions should be created for all removed functions that indicate they will be removed by 0.4. No functionality should be removed before the next release.

Changed 3 years ago by jburke

Addressing your three points, Paul:

1. I will do that.

2. I think I had a problem again with SVN and newly added files. I do have a debug.js that contains dojo.debug() and dojo.debugShallow(). You can see it with the snapshot of my code up here: http://tagneto.org/dojo/bootstrap/src/debug.js. I added the following block to dojo.js (also visible at http://tagneto.org/dojo/bootstrap/dojo.js):

	if((this["djConfig"])&&(djConfig["isDebug"])){
		tmps.push("debug.js");
	}

I think I should also tmps.push("debug.js") if debugAtAllCosts is true. I will add that to the if statement above. How does that sound?

3. I thought one of the points of the cleanup was to remove the functions, and the posts I did to the listservs and dojo blog were to warn people that they were going away. I will follow up in the IRC meeting today to see what I should add back.

Thanks for the review!

Changed 3 years ago by jburke

Attaching new patch file that addresses the following points. Paul Sowden, can you review again?

1. _addHostEnv is wrapped up in a closure so as not to remain in global scope. 2. debug.js will be included if djConfig.isDebug is true or if debugAtAllCosts is true. 3. There is support for a compatibility package. If djConfig.compat = "0.2.2" is set, then dojo.js will load src/compat/0.2.2.js. This file has the function definitions that were removed as part of the bootstrap refactoring.

I will upload a diff file and a tar of the newly added files. You can also find the source and tests online at:

http://tagneto.org/dojo/bootstrap/src and http://tagneto.org/dojo/bootstrap/tests

Changed 3 years ago by jburke

Pass #2 at patch to trunk to refactor bootstrap files.

Changed 3 years ago by jburke

Pass #2 at patch to trunk to refactor bootstrap files (New files)

Changed 3 years ago by jburke

  • status changed from new to closed
  • resolution set to fixed

I'm done with this for now. Any new bootstrap bloat reduction ideas should be filed as new bugs.

Changed 2 years ago by anonymous

  • milestone deleted

Milestone 0.3release deleted

Note: See TracTickets for help on using tickets.