665222 - Deleting all modules from library causes error in editor

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Add-on Builder, defect, P1)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

STR:

1. Using Firefox 4.01, log in to https://builder-addons.allizom.org/
2. Click "Create" and then choose "Library"
3. Delete the index.js file and once that's removed, type in the editor widget

Actual results:

Error: item is null
Source File: https://builder-addons.allizom.org/media/lib/mootools/mootools-core-1.3.2.js
Line: 936

Screencast: http://screencast.com/t/NBtsf4aKDh

Comment 1

[Sean McArthur [:seanmonstar]]

So, when you delete all files, the editor window should become read-only, until a new file is created.

Comment 2

[Daniel Buchner [:dbuc]]

Can we just prevent rename/delete of the index.js file as we do with main.js?

Comment 3

[Sean McArthur [:seanmonstar]]

If we want to enforce that all Libraries have an index.js. It wouldn't change names (otherwise, its too hard to realize why one of your modules can't be deleted). 

We could then add to the package.json of the library that the index.js is the main_module. This would allow require('libname'), and it would load that main_module of that library (currently set to nothing, but could be the index.js file).

Comment 4

[Daniel Buchner [:dbuc]]

Yeah that sounds good, I especially like this part: "This would allow require('libname')". Let's do it, you agree?

Comment 5

[Piotr Zalewa [:zalun]]

I believe the best would be a library to have at least one module

Comment 6

[Sean McArthur [:seanmonstar]]

An example of the downside: your mootools-server has its module also named mootools-server.

If we enforced a required index.js, we would need to add one to any library that doesn't have one. Then, whereas before when require('mootools-server') would first try to load the library, see there was no main_module, and then look for modules and find 'mootools-server', with this it would load the index.js, which could have nothing in it.

Still would be better to make this "breakage" now as opposed to later...

Comment 7

[Piotr Zalewa [:zalun]]

++ as I said - prevent (in JavaScript only) deletion of the last module in library.

Comment 8

[Sean McArthur [:seanmonstar]]

So which is it? All libraries get a permanent index.js module (allowing require('mylib'), but breaking some pre-existing uses), or simply stop deleting modules if module count == 1?

Comment 9

[Piotr Zalewa [:zalun]]

I'd vote for the latter

Comment 10

[Sean McArthur [:seanmonstar]]

I personally vote for the former, because I think being able to require('mylib') to be a beneficial pattern for the Builder.

dbuc, which?

Comment 11

[Daniel Buchner [:dbuc]]

I'd like to see the former, this needs to work: require('mylib');

Comment 12

[Sean McArthur [:seanmonstar]]

Code has landed. https://github.com/mozilla/FlightDeck/commit/53866e1867bade9ab95292a7f03e8bb0ba6295e2

However, the migration fails. @clouserw: what should i change to get this working?

Updater output:

[localhost] out: Running .py migation 11:
[localhost] out: python -B manage.py runscript migrations.011-ensure_library_main_module
[localhost] out: Error: Had trouble running this: python -B manage.py runscript migrations.011-ensure_library_main_module
[localhost] out: stdout: Unknown option: -B
[localhost] out: usage: python [option] ... [-c cmd | -m mod | file | -] [arg] ...
[localhost] out: Try `python -h' for more information.

Comment 13

[Sean McArthur [:seanmonstar]]

Got the migration working. Fixed on master.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED; unable to get UI to delete index.js under "Lib" in a library.