Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CLOSED] download eslint package from npm and place it into default extensions #11121

Open
core-ai-bot opened this issue Aug 30, 2021 · 25 comments
Open

[CLOSED] download eslint package from npm and place it into default extensions #11121

core-ai-bot opened this issue Aug 30, 2021 · 25 comments

Comments

@core-ai-bot
Copy link
Member

Issue by zaggino
Friday Feb 17, 2017 at 01:16 GMT
Originally opened as adobe/brackets#13108

Related to adobe/brackets#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.

zaggino included the following code: https://github.com/adobe/brackets/pull/13108/commits
@core-ai-bot
Copy link
Member Author

Comment by ficristo
Friday Feb 17, 2017 at 08:30 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Friday Feb 17, 2017 at 09:07 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Friday Feb 17, 2017 at 09:16 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Feb 28, 2017 at 06:50 GMT

@ficristo check now

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Mar 01, 2017 at 18:16 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Mar 01, 2017 at 20:42 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Wednesday Mar 01, 2017 at 21:00 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Mar 01, 2017 at 21:09 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Wednesday Mar 01, 2017 at 21:16 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Thursday Mar 02, 2017 at 00:57 GMT

ref: adobe/brackets#13136

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Thursday Apr 13, 2017 at 08:38 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Sunday Apr 16, 2017 at 18:01 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Aug 21, 2017 at 13:12 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Monday Aug 21, 2017 at 13:25 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Aug 21, 2017 at 13:44 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Aug 28, 2017 at 11:38 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Aug 28, 2017 at 11:39 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Monday Aug 28, 2017 at 12:36 GMT

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?

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Aug 28, 2017 at 12:55 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Aug 28, 2017 at 15:01 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Monday Aug 28, 2017 at 21:43 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Tuesday Aug 29, 2017 at 08:05 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Tuesday Aug 29, 2017 at 08:43 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Thursday Aug 31, 2017 at 07:27 GMT

@nethip Can you please review this PR? 🙏

@core-ai-bot
Copy link
Member Author

Comment by nethip
Thursday Aug 31, 2017 at 10:39 GMT

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot