Remove main.js requirement for themes #9434
Conversation
| var mainJS = path.join(extractDir, commonPrefix, "main.js"); | ||
| if (!fs.existsSync(mainJS)) { | ||
| var mainJS = path.join(extractDir, commonPrefix, "main.js"), | ||
| isTheme = checkIfTheme(packageJSON); |
There was a problem hiding this comment.
You can just use metadata here instead of re-parsing it. (Unless metadata is falsy, in which case you should skip the check since the data is unreadable anyway).
So, something like this: var isTheme = metadata && metadata.theme;
There was a problem hiding this comment.
Does metadata exist in that scope? I just read the code again, I don't see any place metadata has been introduced in that scope unless it is coming in through the callback parameter.
There was a problem hiding this comment.
@le717 Yes -- if you look at line 300, metadata is an argument to the function that you're adding this code to.
There was a problem hiding this comment.
Hm. That's interesting, as I am not finding metadata in the script scope, only in individual functions. Will fix this with your suggestion right now.
|
@dangoor should probably do the official review here, but this looks like a decent start to me -- marking as ready for full review (pending my comment above being addressed). |
|
@peterflynn Changes pushed. |
| isTheme = metadata && metadata.theme; | ||
|
|
||
| // Throw missing main.js file only for non-theme extensions | ||
| if (!fs.existsSync(mainJS) && !isTheme) { |
There was a problem hiding this comment.
Checking isTheme first will completely skip checking the file system if the extension is a theme.
|
Changes look good to me. Theme authors will appreciate this one :) |
|
There are jasmine-node tests for package validation. Please add a test to |
|
Also, can you bump the version in the src/extensibility/node/package.json file to 0.45.0? That way, we'll be able to easily install this as a new version for brackets-registry. The change itself seems fine and will be a nice improvement! |
|
@dangoor Changes pushed, ready for further review. I could not find the item in the Jasmine window to run these tests, so can you clue me in on where that link is as well? |
|
Since that's a node part of Brackets, you can run npm install in that directory and also run npm install -g jasmine-node to be able to run Jasmine from the command line. There are some integration tests that run from the test window but these unit tests run from the command line. Sent from my iPhone
|
|
@dangoor All tests pass, including the new one you requested. I'll squash this in a few minutes. :) |
|
That was a long few minutes. Squashed and ready to go. |
|
Looks good to me. Thanks. |
Remove main.js requirement for themes
|
All the theme authors right now. |
This is for #8626.