Remove some circular dependencies #10641

Merged
merged 1 commit into from Mar 6, 2015

Projects

None yet

2 participants

@le717
Contributor
le717 commented Feb 20, 2015

(Branched off discussion in #10600)

  • isCSSPreprocessorFile() -> CSSUtils
  • _isStatic/ServerHtmlFileExt() -> LiveDevelopment
    • Possibly change to be based on language id instead of file ext
  • getSmartFileExtension() -> LanguageManager, and maybe rename to something like getCompoundFileExtension() for clarity
  • Move just showFileOpenError() to DocumentCommandHandlers, next to _showSaveFileError()
  • Update all affected tests/JSDoc

To note, moving showFileOpenError() to DCH and _isStaticHtmlFileExt() to LiveDevelopmentUtils are creating a circular dependencies (sigh) during deprecation.

Once merged, I suggest these APIs are put on the fast track for removal so the cycles they will resolve (and temporarily create/prolong) are removed.

/cc @peterflynn

@peterflynn
Member

isCSSPreprocessorFile() isn't used in any extensions, so I think that one we can flat-out remove now, avoiding the need to deal with a new (albeit temporary) cycle.

@le717
Contributor
le717 commented Feb 20, 2015

Done. I just noticed that _isStatic/ServerHtmlFileExt() is also used by LiveDevMultiBrowser, so relocating them to LiveDevelopment is a no-go, since we don't want one live dev feeding off the other. Maybe the original plan of moving them to LanguageManager will do?

Also, are these functions used by any extensions? I can't check (don't have the catalog on my computer). If not, we can outright move these too.

@peterflynn
Member

@le717 They're very coupled to Live Preview, so I'd like to keep them located there if possible.

The module 'LiveDevelopment/main.js' is a common module between the two impls, but since it depends on them already we'd create a cycle by putting common code there and having the impls depend back on it. There's a tremendous amount of duplicated code between the two impls, so I assume we'll eventually want a common place to put code they both share (unless we're sure the old impl will entirely go away soon, but I don't see a clear path for that yet). Maybe starting a new module like LiveDevelopmentUtils or LiveDevelopmentCore would be worth it.

isStaticHtmlFileExt() is used by three extensions.

@le717
Contributor
le717 commented Feb 20, 2015

While there is indeed a lot of duplicated code, I do not feel comfortable attempting to consolidate that. Already I can see small functions that, if moved, would change their arguments in a way that is probably not wanted. What about making a LiveDevelopmentUtils with those two functions for now and let the multibrowser devs later on clean up and consolidate the duplicated code?

@peterflynn
Member

Yep, that's what I was proposing.

@le717
Contributor
le717 commented Feb 20, 2015

All done, commits flatten too. Ready for final review. :)

@peterflynn peterflynn commented on the diff Mar 4, 2015
test/spec/LanguageManager-test.js
@@ -885,5 +885,45 @@ define(function (require, exports, module) {
expect(LanguageManager.getLanguageForPath("test.abcxyz").isBinary()).toBeFalsy();
});
});
+
+ describe("getCompoundFileExtension", function () {
@peterflynn
peterflynn Mar 4, 2015 Member

I think you still need to remove the original copy of this from FileUtils-test.

@peterflynn peterflynn commented on an outdated diff Mar 4, 2015
src/LiveDevelopment/LiveDevelopmentUtils.js
+ }
+
+ /**
+ * Determine if file extension is a server html file extension.
+ * @param {string} filePath could be a path, a file name or just a file extension
+ * @return {boolean} Returns true if fileExt is in the list
+ */
+ function isServerHtmlFileExt(filePath) {
+ if (!filePath) {
+ return false;
+ }
+
+ return (_serverHtmlFileExts.indexOf(LanguageManager.getLanguageForPath(filePath).getId()) !== -1);
+ }
+
+ function isHtmlFileExt(ext) {
@peterflynn
peterflynn Mar 4, 2015 Member

Nit: add docs

@peterflynn peterflynn commented on an outdated diff Mar 4, 2015
src/language/CSSUtils.js
@@ -62,6 +62,16 @@ define(function (require, exports, module) {
_invertedBracketPairs = _.invert(_bracketPairs);
/**
+ * Determines if file extension is a CSS preprocessor file extension that Brackets supports.
@peterflynn
peterflynn Mar 4, 2015 Member

Now that we've made the coupling more explicit, maybe change to "...that CSSUtils supports"?

Actually, the references to file extension here are misleading since getLanguageForPath() should be called with a fullPath whenever possible, and afaict current callers do always pass a path. So maybe reword the whole thing to: "Determines if the given path is a CSS preprocessor file that CSSUtils supports." And remove the "file name" part from the @param below.

@peterflynn
Member

One last cleanup idea, if you want to add it in: move FileUtils.MAX_FILE_SIZE to AppShellFileSystem, since the limit is really impl-dependent (e.g. in-browser it might be lower). And then in FileUtils.getFileErrorString(), you'd reference it as require("fileSystemImpl").MAX_FILE_SIZE to stay agnostic of which impl is currently in use.

@peterflynn
Member

@le717 Thanks for this! Just a few small changes and then I think it's good to go.

@peterflynn peterflynn self-assigned this Mar 4, 2015
@le717
Contributor
le717 commented Mar 4, 2015

You'd reference it as require("fileSystemImpl").MAX_FILE_SIZE to stay agnostic of which impl is currently in use.

But wouldn't this create a circular dependency, as Bracket's impl uses FileUtils.getNativeBracketsDirectoryPath() and FileUtils.getNativeModuleDirectoryPath()?

Otherwise, review changes pushed.

@le717 le717 Remove some circular dependencies
Move showFileOpenError() to DCH

Update internal calls to getCompoundFileExtension()

Relocate isCSSPreprocessorFile()

Remove isCSSPreprocessorFile() redirect

Update tests

Introduce new LiveDevelopmentUtils module

Redirect/remove _isStatic/ServerHtmlFileExt()

Make _isStatic/ServerHtmlFileExt() use language id
a43a8f7
@le717
Contributor
le717 commented Mar 4, 2015

I was right. Just performed those changes and Brackets locked up, with AppshellFileSystem throwing an "FileUtils is not defined" error, which means a circular dependency has been created (I've seen this error a lot throughout making this PR). Yet again, maybe another issue to file so a good fix can be discussed and put in place?

Ready for merging.

@peterflynn
Member

Ugh, that's a mess -- nothing in the filesystem package was supposed to have any dependencies on the rest of Brackets, originally. Scratch that change then -- leaving the const in FileUtils isn't so bad, and forks with a different fs impl can just change the constant there instead (they will have plenty of other diffs against master anyway).

@peterflynn
Member

Alright, looks great! Seems stable, tests pass -- thanks for cleaning this up!

@peterflynn peterflynn merged commit 5daf5be into adobe:master Mar 6, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@le717 le717 deleted the le717:remove-circular-deps branch Mar 6, 2015
@le717 le717 commented on the diff Mar 6, 2015
src/LiveDevelopment/LiveDevelopmentUtils.js
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define */
+
+/**
+ * Set of utilities for working with files and text content.
@le717
le717 Mar 6, 2015 Contributor

I just realized this module summary is completely wrong.

@peterflynn
peterflynn Mar 6, 2015 Member

Oops, good catch -- feel free to put up a small PR with a fix.

@le717
le717 Mar 7, 2015 Contributor

Will do.

@le717 le717 commented on the diff Mar 6, 2015
src/language/LanguageManager.js
@@ -398,7 +398,40 @@ define(function (require, exports, module) {
function _resetPathLanguageOverrides() {
_filePathToLanguageMap = {};
}
+
+ /**
+ * Get the file extension (excluding ".") given a path OR a bare filename.
+ * Returns "" for names with no extension.
+ * If the only `.` in the file is the first character,
+ * returns "" as this is not considered an extension.
+ * This method considers known extensions which include `.` in them.
+ *
+ * @param {string} fullPath full path to a file or directory
+ * @return {string} Returns the extension of a filename or empty string if
+ * the argument is a directory or a filename with no extension
+ */
+ function getCompoundFileExtension(fullPath) {
+ var baseName = FileUtils.getBaseName(fullPath),
@le717
le717 Mar 6, 2015 Contributor

Funny I didn't catch this earlier. Here we have the proper relationship between LanguageManager and FileUtils (the former relies on the latter), but because of the deprecation warning, there is still a circular loop here but it is not crashing. Perhaps I should submit a quick followup PR to make LanguageManager load FileUtils in async until the deprecation warning is removed to prevent any possible crashes?

@peterflynn
peterflynn Mar 6, 2015 Member

This PR didn't add that cycle -- it already existed. So I don't think any further change is needed here for now.

@le717
Contributor
le717 commented Mar 6, 2015

I guess I should have titled this PR better, as this does not actually remove any circular dependencies. Rather, it moves some things around so the cycles can be removed.

How long do you think the warnings should stay in place? I know extensions need to be updated, but since removing the deprecated things and fixing the require calls will help improve the code base quality, there should be an ideal time for removal. I would say, if possible, v1.4, since I don't think you said too many extensions were affected here.

@peterflynn
Member

We usually leave deprecations for longer than that. It doesn't seem urgent to remove the shims since the existing async-loading workarounds work just fine, and the messy workaround code is very self-contained (i.e. the quality/maintainability impact is quite limited).

@le717
Contributor
le717 commented Mar 7, 2015

Ah, OK. My issue are there is no real guidelines as to when shims/deprecated items should be released. Obviously it will depend on the amount the APIs are used in extensions, but I'd still think there is an estimated/ideal time span before complete removal.

@peterflynn
Member

There's no specific standard timeframe, but I can't think of any case where we removed an API the very next release after it was deprecated. I'd say we usually wait a couple releases, then re-check how many current extensions are still using the APIs (if any) and make a decision from there.

@thehogfather thehogfather added a commit to thehogfather/brackets that referenced this pull request Mar 29, 2015
@thehogfather thehogfather Merge commit 'e56192e09ceed72834985f2483ed70e339e5c519' into codefolding
* commit 'e56192e09ceed72834985f2483ed70e339e5c519':
  Logos are now perfectly centered
  Whitespace trimming suggested by @MarcelGerber
  Fix some moved APIs that were left over from PR #10641
  Text is now perfectly centered
985fe8a
@AJDBenner AJDBenner added a commit to AJDBenner/brackets that referenced this pull request Apr 8, 2015
@peterflynn @AJDBenner peterflynn + AJDBenner Fix some moved APIs that were left over from PR #10641 88ccd2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment