download eslint package from npm and place it into default extensions by zaggino · Pull Request #13108 · adobe/brackets

zaggino · 2017-02-17T01:16:19Z

Related to #11984

As a part of this PR I'd like to make a change, that when brackets-eslint is found in extensions/user directory, it won't be loaded from extensions/default directory - I'd like your thoughts on that.

ficristo · 2017-02-17T08:30:18Z

I think Atom added a new field in their package.json, packageDependencies, and use it to install their extensions. I think I prefer their approach.
For the linters, I still don't understand what Adobe wants to do...

zaggino · 2017-02-17T09:07:31Z

@ficristo Can you shine a bit more light how these packageDependencies should work here? Happy to make changes to the PR

ficristo · 2017-02-17T09:16:54Z

I didn't look at Atom implementation, but I think the usage is similar to the node dependencies.
In the package.json of Atom they have something like:

"packageDependencies": {
  "atom-dark-syntax": "0.28.0",
  "atom-dark-ui": "0.53.0",
  "...": "..."
}

We could have something like:

"dependencies": {
  "chokidar": "1.6.0",
  "...": "..."
},
"devDependencies": {
  "grunt": "0.4.5",
  "...": "..."
},
"packageDependencies": {
  "brackets-eslint": "1.0.0",
  "...": "..."
}

and use npm to install them.
If I understood correctly, you have done this but hardcoded the dependencies in the scripts.
I only think we have to move them on the package.json so to make easier to keep track of what we use.
Of course is totally possible there are some drawback that ATM I don't see.

zaggino · 2017-02-28T06:50:10Z

@ficristo check now

ficristo · 2017-03-01T18:16:55Z

I only skimmed the changes. From what I understood ESLint will installed in the default extensions so users will not able to remove this extension. Is it right?
I prefer if this one will be reviewed by someone at Adobe, sorry.

zaggino · 2017-03-01T20:42:02Z

I don't think that's an issue (same way they can't remove JSLint now nobody really uses), but also a good point, I could make a PR to display default extensions in extension manager and delete them.

redmunds · 2017-03-01T21:00:06Z

@zaggino Do you mean that default extensions would be physically deleted? That might be problematic from install dir due to permissions, and how would user re-install them?

Another option is to list default extensions in extension manager (possibly in a new tab) and allow them to be disabled/re-enabled.

Note that Brackets has not been tested without the default extensions so different combinations should be tested.

zaggino · 2017-03-01T21:09:30Z

@redmunds Yeah I mean't disable/enable as a configuration preference, not sure why I wrote delete.

If some extensions are critical for Brackets to run, we could hard-code that they can't be disabled, but I think most of them won't cause this sort of problems.

redmunds · 2017-03-01T21:16:01Z

Very Cool. I don't think any of the default extensions were intended to be critical -- they just haven't been tested to verify it.

zaggino · 2017-03-02T00:57:35Z

ref: #13136

…gration

zaggino · 2017-04-13T08:38:53Z

default extensions can be now disabled in the UI, so this can take another look guys

ficristo · 2017-04-16T18:01:15Z

I was wondering if instead of downloading from npm wouldn't it be better to use brackets registry?

swmitra · 2017-08-21T13:12:55Z

@zaggino Great work here 👍 . I took the liberty to resolve the conflicts, as I wanted to test this branch. I looked at the code and it looks good to me. If everything works fine in real usage for a couple of days, I will merge this PR. Do you recall any pending item in this PR which I might have missed?

zaggino · 2017-08-21T13:25:42Z

@swmitra The only thing to consider is that after this, it's higly likely that people will have brackets-eslint as a default extension and also as a user extension installed. Only on of those (the user installed version probably) should load when starting up.

swmitra · 2017-08-21T13:44:35Z

@zaggino Thanks a lot for quick response 👍 . I will test that scenario more extensively then.

…ad order

swmitra · 2017-08-28T11:38:32Z

I have modified the registration and fetching logic of registered providers a bit based on the following idiom.

For a given language, we would be picking up only a single language specific linter + any generic linter registered with '*'
Choosing a linter from the registered set of language specific linters depends on priority and load order.

@zaggino @ficristo @petetnt Can you please have a look at this PR urgently? This needs to be in for next release to provide basic es6 support ...

swmitra · 2017-08-28T11:39:18Z

I can no longer review this PR, but definitely work on the PR 😄

zaggino · 2017-08-28T12:36:38Z

I have written most of the code so not much to review.
@swmitra Where's the actual priority being set? The one you use for sorting?

swmitra · 2017-08-28T12:55:15Z

The priority field is for future use. As of now it will default to 0 in the priority check and collision resolution will happen on load order.

swmitra · 2017-08-28T15:01:45Z

@zaggino I should have been more precise. My mistake. I meant the last commit. As the existing code is already reviewed, I wanted to focus just on the open part to handle conflict between same providers. In the process, I thought of limiting the linting by only a single language specific linter. Looking at both EsLint and JSLint together made me skeptical about the overhead.

zaggino · 2017-08-28T21:43:57Z

Not sure about the single linter, on my projects I use both TypeScript (with TSLint) and ESLint to lint my files. I wouldn't want to have only one.

…ost recently loaded one

swmitra · 2017-08-29T08:05:42Z

@zaggino Thanks for sharing the multiple linter use case. Removed the priority based sorting now. Instead, we now look for providers of a given language having same provider name. If found, remove it and retain the most recently loaded one. Tested with same extension co-existence in Brackets core and user exetensions, and what I have noticed is, user extensions are the ones that used for linting if at all there are any name conflicts.
In case someone wants to disable JSLint, it can be done now by just disabling the JSLint extension under default extensions tab in Extension manager.

swmitra · 2017-08-29T08:43:12Z

@petetnt @ficristo Can you please review this PR now? 🙏

swmitra · 2017-08-31T07:27:14Z

@nethip Can you please review this PR? 🙏

nethip · 2017-08-31T07:37:11Z

-      "version": "0.1.4",
-      "from": "xmldoc@>=0.1.2 <0.2.0",
-      "resolved": "https://registry.npmjs.org/xmldoc/-/xmldoc-0.1.4.tgz",
+      "version": "0.1.2",


Version downgrade?

narayani28 · 2017-08-31T10:11:22Z

-      "version": "0.6.1",
-      "from": "sax@>=0.6.1 <0.7.0",
-      "resolved": "https://registry.npmjs.org/sax/-/sax-0.6.1.tgz",
+      "version": "0.4.2",


is this an intended downgrade of the version?

nethip · 2017-08-31T10:39:21Z

@swmitra Code wise I am fine. I have taken the liberty in adding check boxes for pending items, so that it is easier to track the progress.

Build this branch on all the three platforms (Win, Mac, Linux), locally and see that you are able to succesfully use the www folder, built from this branch.
Run smokes on all the platforms, as the library versions have changed for some
Trigger installer builds on our build system to see for all the platforms if the build is going fine
Have a look out for app footprint size
Update documentation wherever required

Update - Library versions are intact now.
Built 'www' size increased by ~400KB
No Documentation update required now. But we have to mention about how to disable JSLint, when releasing Brackets.

narayani28

LGTM

petetnt

FWIW LGTM, bit too late though. Nice work!

…adobe#13108) * download eslint package from npm and place it into default extensions * add defaultExtensions section to package.json * lock deps, add shrinkwrap again * do not lint brackets-eslint files * Handle multiple linters for a specific language using priority and load order * Handle provider name conflict for a given language by retaining the most recently loaded one * Remove invalid comment * Update npm-shrinkwrap.json

zaggino added the Enhancement label Feb 17, 2017

zaggino self-assigned this Feb 17, 2017

zaggino requested review from ficristo, petetnt and swmitra February 17, 2017 01:16

zaggino added 2 commits February 28, 2017 17:43

download eslint package from npm and place it into default extensions

9a0e2f8

add defaultExtensions section to package.json

3e45509

zaggino force-pushed the zaggino/eslint-integration branch from bb72d91 to 3e45509 Compare February 28, 2017 06:43

lock deps, add shrinkwrap again

16d5f2e

ficristo removed their request for review March 1, 2017 18:17

zaggino mentioned this pull request Mar 2, 2017

add a tab with default extensions to extension manager #13136

Merged

zaggino added 2 commits April 13, 2017 18:27

Merge remote-tracking branch 'origin/master' into zaggino/eslint-inte…

f4d9e9d

…gration

do not lint brackets-eslint files

ebf5209

Merge branch 'master' into zaggino/eslint-integration

d344697

Handle multiple linters for a specific language using priority and lo…

3ff8dbe

…ad order

swmitra removed their request for review August 28, 2017 11:39

Handle provider name conflict for a given language by retaining the m…

eaa51e2

…ost recently loaded one

Remove invalid comment

71030bc

swmitra requested a review from ficristo August 29, 2017 08:43

swmitra requested a review from nethip August 31, 2017 07:27

nethip reviewed Aug 31, 2017

View reviewed changes

narayani28 reviewed Aug 31, 2017

View reviewed changes

Update npm-shrinkwrap.json

462729a

narayani28 approved these changes Aug 31, 2017

View reviewed changes

saurabh95 self-requested a review August 31, 2017 15:14

saurabh95 approved these changes Aug 31, 2017

View reviewed changes

saurabh95 merged commit 764fb02 into master Aug 31, 2017

petetnt reviewed Aug 31, 2017

View reviewed changes

petetnt deleted the zaggino/eslint-integration branch August 31, 2017 21:37

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] download eslint package from npm and place it into default extensions brackets-archive/bracketsIssues#11121

Open

Conversation

zaggino commented Feb 17, 2017

Uh oh!

ficristo commented Feb 17, 2017

Uh oh!

zaggino commented Feb 17, 2017

Uh oh!

ficristo commented Feb 17, 2017

Uh oh!

zaggino commented Feb 28, 2017

Uh oh!

ficristo commented Mar 1, 2017

Uh oh!

zaggino commented Mar 1, 2017

Uh oh!

redmunds commented Mar 1, 2017

Uh oh!

zaggino commented Mar 1, 2017

Uh oh!

redmunds commented Mar 1, 2017

Uh oh!

zaggino commented Mar 2, 2017

Uh oh!

zaggino commented Apr 13, 2017

Uh oh!

ficristo commented Apr 16, 2017

Uh oh!

swmitra commented Aug 21, 2017

Uh oh!

zaggino commented Aug 21, 2017

Uh oh!

swmitra commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swmitra commented Aug 28, 2017

Uh oh!

swmitra commented Aug 28, 2017

Uh oh!

zaggino commented Aug 28, 2017

Uh oh!

swmitra commented Aug 28, 2017

Uh oh!

swmitra commented Aug 28, 2017

Uh oh!

zaggino commented Aug 28, 2017

Uh oh!

swmitra commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swmitra commented Aug 29, 2017

Uh oh!

swmitra commented Aug 31, 2017

Uh oh!

nethip Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

narayani28 Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

nethip commented Aug 31, 2017 • edited by swmitra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

narayani28 left a comment

Choose a reason for hiding this comment

Uh oh!

petetnt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

swmitra commented Aug 21, 2017 •

edited

Loading

swmitra commented Aug 29, 2017 •

edited

Loading

nethip commented Aug 31, 2017 •

edited by swmitra

Loading