search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] [CEF 2171] JSLint doesn't work #8856

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by JeffryBooher Tuesday Nov 18, 2014 at 22:31 GMT Originally opened as https://github.com/adobe/brackets/issues/9966

extensions/default/jslint/main.js:101 JSLINT is undefined.

core-ai-bot commented 3 years ago

Comment by peterflynn Wednesday Nov 19, 2014 at 22:19 GMT

The problem is a bug in the (fairly old) version of JSLint that we use. This line is trying to add an expando property to a string, which is illegal -- it used to fail silently, but newer Chrome now throws an error for. The code should be assigning that property onto a different object, as fixed in newer versions of JSLint.

Unfortunately it's not easy to skip ahead to the latest JSLint because it's added tons of new warnings that cannot be disabled, requiring lots of disruption to the Brackets codebase to stay JSLint-clean. So we may need to fork and patch, as we have with a few other third-party libs.

Anyone testing CEF can easily fix this locally though -- just change s to x on that line.

core-ai-bot commented 3 years ago

Comment by peterflynn Wednesday Nov 19, 2014 at 22:20 GMT

OTOH there are only 39 commits between our current version of JSLint and the fix, so it might not be too crazy to upgrade just that far: https://github.com/douglascrockford/JSLint/compare/2347ec4...656150a46106206b915e1e881ce95e2ce0c12cd2

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Nov 20, 2014 at 05:16 GMT

Unfortunately, that diff range has the dreaded 'unexpected else' feature added, which was one of the blockers last time we looked at updating JSLint. Looks like forking is probably the simplest option...

core-ai-bot commented 3 years ago

Comment by le717 Thursday Nov 20, 2014 at 21:40 GMT

@peterflynn Could this be a good time to switch to JSHint by default, considering all code already passes that (as indicated by the Travis builder)?

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Nov 21, 2014 at 04:06 GMT

@le717 The problem is that JSHint has dropped support for checking code formatting/whitespace, which we still consider important. We'd have to either integrate an older version of JSHint (2.4 or earlier), or integrate both JSHint and a separate style-checking tool like JSCS (which some people have said is much slower -- problematic for the way Brackets auto-runs the linter, especially if we want to upgrade to lint-as-you-type in the future).

There's a little discussion of this in Trello, too.

core-ai-bot commented 3 years ago

Comment by le717 Friday Nov 21, 2014 at 04:46 GMT

@peterflynn Has ESLint been considered? http://eslint.org It can be fully configured, seems to have many formatting options, and IIRC there are JSLint and JSHint configuration defaults that can be installed. IMO, JSLint is getting too strict and unconfigrable, but JSHint dropping formatting was not a move I am happy about either. There has to be a customizable medium somewhere that doesn't have as bad as tradeoffs as the newest JSLint or JSHint + JSCS...

core-ai-bot commented 3 years ago

Comment by dangoor Friday Nov 21, 2014 at 13:29 GMT

ESLint would be a viable option from what I know of it, but there is still likely some work required to get there. For this particular issue, our main goal is to unblock the landing of a newer CEF (Chromium) for the shell and for that purpose, it's simpler to just fork JSLint.

I totally agree, though, that JSLint is getting too old and gnarly and needs to go (as does everyone else who works on Brackets, I'd imagine ;)

core-ai-bot commented 3 years ago

Comment by le717 Friday Nov 21, 2014 at 19:23 GMT

Fix for this is up at #9994. I'm (obviously) agreeing with you, let's just fork it for now so it works with the new CEF.

core-ai-bot commented 3 years ago

Comment by peterflynn Wednesday Dec 03, 2014 at 19:25 GMT

Posted a fix in PR #10077 that uses a fork, to keep the repo history clear.