New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overriding JS linting doesn't work on startup #5442

Closed
cfjedimaster opened this Issue Oct 7, 2013 · 10 comments

Comments

Projects
None yet
2 participants
@cfjedimaster
Contributor

cfjedimaster commented Oct 7, 2013

My JSHint extension (about to be checked in with an update in the next five minutes) makes use of the new Linting API. If Brackets launches with a JS file as the default open document, the linting will be done with JSLint. If I edit the file, it will switch to JSHint and stay that way for the rest of the editing session.

@ghost ghost assigned peterflynn Oct 7, 2013

@cfjedimaster

This comment has been minimized.

Show comment
Hide comment
@cfjedimaster

cfjedimaster Oct 8, 2013

Contributor

I'm now seeing this with CSSLinter as well. If a CSS file is the active doc on startup, it will show that it has no linter until I edit the file.

Contributor

cfjedimaster commented Oct 8, 2013

I'm now seeing this with CSSLinter as well. If a CSS file is the active doc on startup, it will show that it has no linter until I edit the file.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 8, 2013

Member

It works fine for me, but I suspect the problem here is a race condition: ExtensionLoader loads core extensions in parallel with user-installed/dev extensions. We still start loading the core extensions first, so normally they're likely to finish sooner too... but there's no guarantee. Which means in some cases it's possible for the core JSLint extension to load after a user-installed extension that wants to override it.

Workaround -- You can work around this by waiting for AppInit.appReady() to register your provider. There's a risk that the user's first file will be opened before then (and thus inspected with the default linter that was registered earlier). You can work around that by kicking a re-lint via CodeInspection.toggleEnabled() after you register.

But ideally you could just be guaranteed that default extensions all load first. It would have other benefits like more consistent menu ordering as well. I'll work on this.

Member

peterflynn commented Oct 8, 2013

It works fine for me, but I suspect the problem here is a race condition: ExtensionLoader loads core extensions in parallel with user-installed/dev extensions. We still start loading the core extensions first, so normally they're likely to finish sooner too... but there's no guarantee. Which means in some cases it's possible for the core JSLint extension to load after a user-installed extension that wants to override it.

Workaround -- You can work around this by waiting for AppInit.appReady() to register your provider. There's a risk that the user's first file will be opened before then (and thus inspected with the default linter that was registered earlier). You can work around that by kicking a re-lint via CodeInspection.toggleEnabled() after you register.

But ideally you could just be guaranteed that default extensions all load first. It would have other benefits like more consistent menu ordering as well. I'll work on this.

@cfjedimaster

This comment has been minimized.

Show comment
Hide comment
@cfjedimaster

cfjedimaster Oct 8, 2013

Contributor

I am currently using AppInit.appReady() though. For both jsHint and my CSS Linter. Oh, you meant use that and triggering a re-lint.

So are you seriously saying to just do this:

AppInit.appReady(function () {

    CodeInspection.register("javascript", {
        name: "JSHint",
        scanFile: handleHinter
    });
    //NEW LINE
   CodeInspection.toggleEnabled();
});
Contributor

cfjedimaster commented Oct 8, 2013

I am currently using AppInit.appReady() though. For both jsHint and my CSS Linter. Oh, you meant use that and triggering a re-lint.

So are you seriously saying to just do this:

AppInit.appReady(function () {

    CodeInspection.register("javascript", {
        name: "JSHint",
        scanFile: handleHinter
    });
    //NEW LINE
   CodeInspection.toggleEnabled();
});
@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 8, 2013

Member

@cfjedimaster Yes, as a workaround until this bug is fixed.

But in the case of CSS Linter you don't have any reason to wait until appReady since that itself is a workaround for (reliably) overriding the default JSLint extension. So for CSS you can just call register() at the top level with nothing else needed.

Member

peterflynn commented Oct 8, 2013

@cfjedimaster Yes, as a workaround until this bug is fixed.

But in the case of CSS Linter you don't have any reason to wait until appReady since that itself is a workaround for (reliably) overriding the default JSLint extension. So for CSS you can just call register() at the top level with nothing else needed.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 8, 2013

Member

Actually correction, call toggleEnabled() twice -- so you're just kicking it to update, not actually changing the final state of the enabled flag.

Member

peterflynn commented Oct 8, 2013

Actually correction, call toggleEnabled() twice -- so you're just kicking it to update, not actually changing the final state of the enabled flag.

@cfjedimaster

This comment has been minimized.

Show comment
Hide comment
@cfjedimaster

cfjedimaster Oct 8, 2013

Contributor

I can confirm this mod works for CSSLint. Testing JSHint now.

Contributor

cfjedimaster commented Oct 8, 2013

I can confirm this mod works for CSSLint. Testing JSHint now.

@cfjedimaster

This comment has been minimized.

Show comment
Hide comment
@cfjedimaster

cfjedimaster Oct 8, 2013

Contributor

Ditto for JSHint. Releasing an update for em now. Thank you.

Contributor

cfjedimaster commented Oct 8, 2013

Ditto for JSHint. Releasing an update for em now. Thank you.

peterflynn added a commit that referenced this issue Oct 10, 2013

Wait until all core extensions are loaded before loading user extensi…
…ons.

This makes menu ordering more consistent, and fixes bug #5442 where the
order of registration matters.

Also change extension loading to work with arrays most of the time, avoiding
bugs if there are ","s in pathnames.

And fix stale comment in ExtensionManager.
@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 10, 2013

Member

@cfjedimaster I have a branch where loading order is more predictable. Would you mind checking on pflynn/load-core-exts-first to see if your extensions work correctly without all the above workarounds? (You shouldn't need to wait for appReady() anymore).

Member

peterflynn commented Oct 10, 2013

@cfjedimaster I have a branch where loading order is more predictable. Would you mind checking on pflynn/load-core-exts-first to see if your extensions work correctly without all the above workarounds? (You shouldn't need to wait for appReady() anymore).

@cfjedimaster

This comment has been minimized.

Show comment
Hide comment
@cfjedimaster

cfjedimaster Oct 13, 2013

Contributor

I just tested with JSHint and it seems to be working as you said (removed
appReady and the toggleEnabled hack).

On Wed, Oct 9, 2013 at 7:03 PM, Peter Flynn notifications@github.comwrote:

@cfjedimaster https://github.com/cfjedimaster I have a branch where
loading order is more predictable. Would you mind checking on
pflynn/load-core-exts-first to see if your extensions work correctly *
without* all the above workarounds? (You shouldn't need to wait for
appReady() anymore).


Reply to this email directly or view it on GitHubhttps://github.com//issues/5442#issuecomment-26019488
.

Raymond Camden, Adobe Developer Evangelist

Email : raymondcamden@gmail.com
Blog : www.raymondcamden.com
Twitter: cfjedimaster

Contributor

cfjedimaster commented Oct 13, 2013

I just tested with JSHint and it seems to be working as you said (removed
appReady and the toggleEnabled hack).

On Wed, Oct 9, 2013 at 7:03 PM, Peter Flynn notifications@github.comwrote:

@cfjedimaster https://github.com/cfjedimaster I have a branch where
loading order is more predictable. Would you mind checking on
pflynn/load-core-exts-first to see if your extensions work correctly *
without* all the above workarounds? (You shouldn't need to wait for
appReady() anymore).


Reply to this email directly or view it on GitHubhttps://github.com//issues/5442#issuecomment-26019488
.

Raymond Camden, Adobe Developer Evangelist

Email : raymondcamden@gmail.com
Blog : www.raymondcamden.com
Twitter: cfjedimaster

@ghost ghost assigned cfjedimaster Oct 22, 2013

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 22, 2013

Member

@cfjedimaster Glad to hear it! The fix has landed in master now, so closing.

Member

peterflynn commented Oct 22, 2013

@cfjedimaster Glad to hear it! The fix has landed in master now, so closing.

@peterflynn peterflynn closed this Oct 22, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment