[CLOSED] Remove some circular dependencies

[core-ai-bot]

Issue by le717
Friday Feb 20, 2015 at 18:32 GMT
Originally opened as adobe/brackets#10641

---

(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

---

le717 included the following code: https://github.com/adobe/brackets/pull/10641/commits

[core-ai-bot]

Comment by peterflynn
Friday Feb 20, 2015 at 19:28 GMT

---

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.

[core-ai-bot]

Comment by le717
Friday Feb 20, 2015 at 20:16 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Friday Feb 20, 2015 at 20:29 GMT

---

@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.

[core-ai-bot]

Comment by le717
Friday Feb 20, 2015 at 20:50 GMT

---

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?

[core-ai-bot]

Comment by peterflynn
Friday Feb 20, 2015 at 20:56 GMT

---

Yep, that's what I was proposing.

[core-ai-bot]

Comment by le717
Friday Feb 20, 2015 at 21:30 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Wednesday Mar 04, 2015 at 23:35 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Wednesday Mar 04, 2015 at 23:35 GMT

---

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

[core-ai-bot]

Comment by le717
Wednesday Mar 04, 2015 at 23:43 GMT

---

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.

[core-ai-bot]

Comment by le717
Wednesday Mar 04, 2015 at 23:55 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Thursday Mar 05, 2015 at 01:37 GMT

---

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).

[core-ai-bot]

Comment by peterflynn
Friday Mar 06, 2015 at 06:20 GMT

---

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

[core-ai-bot]

Comment by le717
Friday Mar 06, 2015 at 15:54 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Friday Mar 06, 2015 at 23:59 GMT

---

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).

[core-ai-bot]

Comment by le717
Saturday Mar 07, 2015 at 00:06 GMT

---

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.