search

adopted-ember-addons / ember-electron

:zap: Build, test, compile and package desktop apps with Ember and Electron
https://ember-electron.js.org/
Other
805 stars 111 forks source link

Add eslint env/config for ember-electron projects #357

Open jacobq opened 6 years ago

jacobq commented 6 years ago

Would it make sense to make an eslint plugin that would get added during ember install ember-electron? Mainly, it'd be nice not to have to manually fix the error 'requireNode' is not defined no-undef entries, but perhaps there are other things to note.

jacobq commented 6 years ago

Is there a reason we're running standard to check mocha test style vs. doing this as part of an eslint plugin like eslint-plugin-mocha? cc @pichfl cc @felixrieseberg

jacobq commented 6 years ago

Rather than an eslint plugin, perhaps we could just add some rules in the override section. This change might fit together nicely with the proposal of removing the .eslintrc in the blueprint.

jacobq commented 6 years ago

Also, it looks like the electron-out folder is not necessarily ignored by eslint (runs on the project root), so we may want to consider adding a line to .eslintignore to exclude it.

bendemboski commented 4 years ago

Much of this is fixed in v3, however we kinda punted on linting. It does lint out-of-box, but we added electron-app to .eslintignore and just don't lint that project.

At some point I do think it would make sense to revisit, and figure out a good default linting config for the electron-app project. On the other hand, electron-forge doesn't have any linting config, and I was unable to find much in the way of established practices for Electron projects...so not sure if we should just let people roll their own until/unless the Electron community starts putting together some linting practices.

RobbieTheWagner commented 4 years ago

I would argue for adding eslint-plugin-node, like ember does by default. Thoughts?

jacobq commented 4 years ago

Before suggesting solutions, perhaps we could try to make a list of the main goals / pain-points we'd like linting to help with. Things that come to mind immediately are processNode and requireNode.

If there isn't already a set of linting rules for electron, we might want consider creating a separate package for that and configuring ember-electron projects to use them for JS files in electron-app. In particular, I'm thinking of warning about use of remote after reading "Electron’s ‘remote’ module considered harmful".

bendemboski commented 4 years ago

TL;DR: I think we should write up an FAQ on linting, and leave the blueprint as-is.

My opinion here is that we should keep ember-electron focused on its core job, which is to integrate Ember with Electron. I think this involves three goals:

  1. Instrument Ember framework/runtime as needed to support integration (e.g. the require/requireNode stuff to allow node integration to coexist with Ember's loader.js, or the electron-protocol-serve stuff to allow Ember's routing to work when loading from the filesystem rather than http: URLs
  2. Allow Ember tooling (ember-cli) to coexist with Electron tooling (electron-forge) to create an integrated development environment (e.g. ember electron:* commands)
  3. Provide a good "quickstart" experience and good documentation to support Ember developers to learn about Electron using their experience with Ember as a starting point

Feel free to suggest other goals I'm maybe not thinking of.

If someone were to make a feature request that has nothing to do with the above, but is entirely in the domain of Electron/electron-forge (e.g. requesting a way to determine from main process code if we're running is a development environment or in a packaged app), I would argue for at most adding info to an FAQ or something, but not trying to fix things that don't have anything to do with the Ember/Electron integration. This is a similar philosophy to what Ember has been doing recently in removing stuff like Ember.Evented that doesn't have anything to do with Ember's core value proposition, or the long-standing philosophy of putting things in addons when it's possible and there isn't a really really good argument for putting it in the core.

linting is kind of a tricky one, and we can definitely argue about it -- Ember provides out-of-box linting, so should we just be making sure it doesn't break the Electron project (which is what we're doing now by simply ignoring it in .eslintrc.js), or should we call full linting an "integrated development environment" feature and try to actually lint the Electron project?

The linting story in the Electron ecosystem seems pretty un-developed. Try a google search and you'll mostly find results geared towards contributing to the Electron project itself. electron-forge has a lint command, but it's just a thin wrapper around yarn-or-npm lint, and the default electron-forge template configures the lint script to be a no-op.

So I'm nervous about trying to forge (heh) our own path linting Electron projects, because I don't think we have enough experience with this new project layout to have a good sense of the right direction to go, and we have no guidance from Electron/electron-forge, nor can we predict what direction they will go in to figure out how that would fit into our ember-electron integrated developer environment. So I think we're best off not adding this to our "API surface area" until we know more, and maybe our efforts would best be spent building up some tooling focused on Electron apps (e.g. eslint-plugin-electron or something) to help move the Electron ecosystem forward into a world with a good linting story.

If y'all agree, I'd be happy to write up an FAQ on linting, with my config as an example (I do have full linting working in my ember-electron app) so we can point people there and then start collecting some usage results.

RobbieTheWagner commented 3 years ago

@bendemboski I think documenting would be an adequate fix here. Linting is very personal to most folks anyway, so forcibly editing their config may not be what we want anyway.