search

cfjedimaster / brackets-csslint

CSSLint extension for Brackets
62 stars 13 forks source link

Extension prevents Adobe Extract from loading #34

Closed le717 closed 9 years ago

le717 commented 9 years ago

See also: adobe/brackets#10314.

Not sure if there anything you can do or if this must be fixed on CSSLint's side, but the version of CSSLint included in f31363f9923da4141765972ecf9c7b44d178edfc and up prevents the Extract extension from loading.

cfjedimaster commented 9 years ago

boggle. Can I blame Extract? ;)

le717 commented 9 years ago

Haha.

If you take a look at this, you'll see that CSSLint removed commented out exports code. I am thinking that is the issue root. The question is, how to work around it?

cfjedimaster commented 9 years ago

If I remember right, the earlier version didn't work well in Brackets as a module.

le717 commented 9 years ago

I am not sure what is happening here. I thought it might have been how CSSLint is loading without assigning a variable (https://github.com/cfjedimaster/brackets-csslint/blob/728e165bdbdbc3ab41e1ee27c6521e8d24bd29e8/main.js#L13), but that did not help anything, and I am not thinking this is an error on Extract's side either.

cfjedimaster commented 9 years ago

Honestly not sure what to suggest here. I'll bring it up to Brackets dev.

le717 commented 9 years ago

The broken CSSLint version is having cascading effects.

dnbard/brackets-extension-rating#101 asgerf/bracket-rename#6

And possibly more extensions.

It may be best to release a version that reverts to the CSSLint at 728e165bdbdbc3ab41e1ee27c6521e8d24bd29e8 then this issue can be further explored.

Did you bring this up to brackets-dev yet?

cfjedimaster commented 9 years ago

I brought it up to NJ on the team. I'll try to handle this tomorrow.

On Tue, Jan 20, 2015 at 7:41 PM, Triangle717 notifications@github.com wrote:

The broken CSSLint version is having cascading effects.

dnbard/brackets-extension-rating#101 https://github.com/dnbard/brackets-extension-rating/issues/101 asgerf/bracket-rename#6 https://github.com/asgerf/bracket-rename/issues/6

And possibly more extensions.

It may be best to release a version that reverts to the CSSLint at 728e165 https://github.com/cfjedimaster/brackets-csslint/commit/728e165bdbdbc3ab41e1ee27c6521e8d24bd29e8 then this issue can be further explored.

Did you bring this up to brackets-dev yet?

— Reply to this email directly or view it on GitHub https://github.com/cfjedimaster/brackets-csslint/issues/34#issuecomment-70770219 .

Raymond Camden, Web Standards Evangelist

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

jasonsanjose commented 9 years ago

Looks like this was filed in the Brackets repo here https://github.com/adobe/brackets/issues/10314.

cfjedimaster commented 9 years ago

I'm checking in a fix now. Well a hack - a hack I used before. I'll update the Brackets bug w/ info on the fix. I really want to understand the why here. :)

On Thu, Jan 22, 2015 at 2:41 AM, Jason San Jose notifications@github.com wrote:

Looks like this was filed in the Brackets repo here adobe/brackets#10314 https://github.com/adobe/brackets/issues/10314.

— Reply to this email directly or view it on GitHub https://github.com/cfjedimaster/brackets-csslint/issues/34#issuecomment-70987791 .

Raymond Camden, Web Standards Evangelist

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

jasonsanjose commented 9 years ago

The why is likely due to the use of exports and how requirejs tries to load non-AMD modules.

peterflynn commented 9 years ago

@jrburke Wondering if you might have any words of wisdom on this :-)

Any idea why var exports = exports || {}; (in csslint.js, which lacks an AMD wrapper), when loaded via require() in a module that does have an AMD wrapper (like this), would cause Require to get into a bad state and fail to load other unrelated modules? Is that a bug in Require or is this csslint.js library doing something badly behaved?

More info is in this bug: adobe/brackets#10314. Note that separate Brackets extensions are loaded in separate Require config contexts (here's the code), which I'd think would make them extra isolated... but this problem with csslint.js cascades over and causes a module-loading glitch in a different extension's config/context. The undefined _ identifier seen in that bug's error message is expected via "shim": { "underscore": { "exports": "_" }, ... } in that other extension's Require config context.

(We've seen one other case where Require errors in one extension cascaded into another extension -- https://github.com/adobe/brackets/issues/8445#issuecomment-49686342 -- which maybe means we're not setting up those "isolated" contexts quite correctly. But there the initial error was more obvious, whereas in this bug it's more of a mystery why loading would csslint.js break anything at the start).

CC @jasonsanjose

jrburke commented 9 years ago

@peterflynn: my first guess is that if you are using cjsTranslate: true with the r.js optimizer to build things, or some other tool that detects CommonJS modules and AMD-wraps them, then it is likely that the module then is wrapped in a define() that gets an exports object, and there is probably some craziness that stems from that.

The CJS detection basically looks for things like require, exports, module, and if those variables are used a wrapping is assumed, so it is no longer a "browser globals" script when it gets inlined in a build.

If that does not sound like a possibility, if I can get some pointers to how to reproduce/debug (new to brackets dev), then I can take more of a look.