Relocate store.server.js and related files/tests. #673

Merged
merged 15 commits into from Oct 29, 2012

Projects

None yet

3 participants

@mojit0
Contributor
mojit0 commented Oct 26, 2012

Moved store.server.js and package-walker.server.js to app/autoload where they're more appropriate.
Added new store.js file at the top level, and updated mojito.js to rely on it to configure the store instance.
Migrated all tests related to the relocated files to their proper location in the test hierarchy.
Patched a minor glitch (no mojito-assets-addon in requires[]) in the default archetypes for mojit which caused new mojits to fail on startup.
Patched start.js command output to avoid using localhost and instead to rely on 127.0.0.1 which works with or without an /etc/hosts entry or similar mapping for localhost.

mojit0 added some commits Oct 26, 2012
@mojit0 mojit0 Fix localhost reference.
With new startup logic the proper reference is 127.0.0.1 so you can click on the log entry and have it work consistently.
cef2274
@mojit0 mojit0 Fix buried error message.
The true error is masked here. Repaired to always report actual error, which can come from an inadequately configured sandbox object etc.
9c9c7c4
@mojit0 mojit0 Fix missing assets addon in mojit archetypes.
The assets add-on is needed for the addCss call in the default and full archetypes, but isn't included by default so simple new mojits will fail.
c7db5ad
@mojit0 mojit0 Refactor initial resource store creation.
Migrated the initial logic for resource store creation out of the mojito.js file and into a store.js file. This secondary file creates the  YUI sandbox and store, getting it ready for use.
c0b5900
@mojit0 mojit0 Remove debugging console call. c1969b9
@mojit0 mojit0 Relocate store.server.js to app/autoload.
Moved store.server.js and related package-walker.server.js to the app/autoload directory.
Adjusted store.server.js to have better defaulting when config isn't complete or isn't provided.
Repaired sandbox limitations in the yui.server.js add-on (require and module aren't part of default vm context).
fad55ba
@mojit0 mojit0 Add missing file. 17461cf
@mojit0 mojit0 Remove debug logging calls. a47bc71
@mojit0 mojit0 Remove lint. 2e788bf
@mojit0 mojit0 Migrate test files for relocated lib files.
Moved the store.server.js and package-walker.server.js files so we're relocating their respective test files and descriptors.
6c1568c
@mojit0 mojit0 Fix broken test.
The configuration test data was out of sync with the fixture application.json content so the test was failing.
6d61634
@caridy

I don't think we should create a store instance everytime. When I do require('store') that should probably give me the store instance already created.

Owner

I disagree. If we combine require() with create() that conflates two operations and makes it much harder for us to manage options. I believe the consumer of the store should be in control of when the instance is created so they can properly manage options construction etc.

I think you guys are talking past each other. I think Caridy isn't proposing that require('store') creates a store, merely that it returns a store. Something somewhere else actually creates the store, and somehow stashes it somewhere so that require('store') will return that instance.

Owner

For now I'd prefer to leave this PR focused on the first step, which was to relocate the files and get the knowledge around creating the YUI sandbox out of the mojito.js file. If we want to migrate functionality between store.js and app/autoload/store.server.js after this that can easily be handled in a followup PR.

Yes, that's what I mean, but I think what @mojit0 is saying is that each time we do require('store') it should create a new instance completely isolated, that's the part I disagree with due to the nature of the store, it should never change, and it is a living object that will be growing as new requests comes in and expanding new mojit instances and more contexts.

Owner

Actually what I'm saying is require() should not create instances. If require creates the instance how do the options get configured? In my mind it's just a bad idea to have require() do anything other than provide a handle to the factory, which can then be used by the consumer to drive configuration of the instance to be created.

@caridy

Since this routine is part of the createStore, that means everytime you call for require('store') it will create a new instance, which is not what we want. Instead, it should create the instance once (the first time it is required), then handing over the same instance over and over again since it is persistent.

Owner

My understanding is that we want the store to have it's own sandbox. If we were to create multiple stores we'd need multiple sandboxes, one per store, to avoid metadata corruption.

@caridy

Export the actual store instance instead.

Owner

See previous comments. I don't think require() should create an instance, or presume that we can only have a single instance of one.

Collaborator
caridy commented Oct 27, 2012

We will also need to update all the commands since most of them uses the store. This is kind of challenging because they might also use YUI (in a very minimum way) but we might need to do some clean up I think.

Contributor
mojit0 commented Oct 29, 2012

@caridy re: the commands, yes. I'll do that today, along with untangling the conflict re: Drew's unit test fixes, and update the PR.

Collaborator
caridy commented Oct 29, 2012

+1

we can refine the factory later on.

@mojit0 mojit0 merged commit 8ffe620 into yahoo:develop-perf Oct 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment