Instruct ESLint to allow conditional imports

[rhettlivingston]

TODO:

• If this is a significant change, update CHANGELOG.md
• Use <h2 id="foo"> instead of ## Foo for headers
• Leave a blank line after each header

Conditional imports as now enabled by Ben's changes in 1.3.3 and beyond cause the default configuration of ESLint to emit a syntax error and stop further analysis.

Thankfully, the maintainers of babel-eslint, one of the parsers that works with ESLint, accepted a PR to allow us to fix this.

The changes to the ESLint setup in this commit simply enable that fix.

[tmeasday]

Oh, nice! can we get a PR to todos?

[rhettlivingston]

Coming tonight.

On Thu, Jun 30, 2016 at 7:21 PM, Tom Coleman notifications@github.com
wrote:

Oh, nice! can we get a PR to todos?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#497 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AH5TYsWtG9c4qIAys0QserFCk2nL4Xccks5qRE-GgaJpZM4JCZKr
.

[rhettlivingston]

Wow, famous last words. I have been working with the 1.4 branch of meteor for two months and had no idea that the latest versions of the eslint-import-plugin are broke under node 0.10.45.

I realized that some lint errors weren't getting produced and tried to figure out why. In doing so, I found that we can't update the plugins without breaking ESLint. They work fine if I run with node 4 (and show a few issues in todos), but an exception is produced in node 0.10.45.

I'm worried now that the ESLint configuration we're showing in the guide is a Meteor 1.4 only configuration.

[tmeasday]

I'm pretty sure what's in the guide right now is also in todos, although I could be wrong about that.

[rhettlivingston]

It isn't. A combination of a missing devDependency and old NPM package versions made it so the checks for whether imports were resolved were not being performed at all. I created an issue in todos and submitted the PR to fix it. As soon as that is pulled in, I'll go for this next update.

This next update would really be of best effect if combined with an update to Meteor 1.3.4.1 and a change to actually uses nested imports in the two files that currently have requires inside conditionals.

Of course, Windows users might be frustrated by that.

Should I go for it or just fix the ESLint configuration?

Also, I'm of the opinion that ESLint configuration should be done in .eslintrc.json instead of package.json just because ESLint allows comments to appear in .eslintrc.json. What are your thoughts on breaking it out?

[tmeasday]

Of course, Windows users might be frustrated by that.

Why do you say that?

Should I go for it or just fix the ESLint configuration?

I say go for it!

Also, I'm of the opinion that ESLint configuration should be done in .eslintrc.json instead of package.json just because ESLint allows comments to appear in .eslintrc.json. What are your thoughts on breaking it out?

I'm hoping that we can update the upstream eslint packages so we don't have to have too complicated a section in package.json. It should be simple to get a new project going with the same settings. If it's not, we should do something about it!

[rhettlivingston]

Of course, Windows users might be frustrated by that.

Why do you say that?

I'm STILL seeing Windows users complaining in various forums of not being able to update. Can't get clear patterns from the noise but suspect it is a combination of

• the problems we've all seen with the downloads freezing (I'm on Ubuntu and see them sometimes though I just notice that the network traffic went flat and restart it, after which it blasts on through) and
• having one of the versions that was broken on their machine and it doesn't work well enough to update.

So my thought was having the todos require 1.3.4.1 could exacerbate the problems that new windows users have. But, I believe in moving forward through a wreck, not hitting the brakes. 😄

I say go for it!

Pedal to the metal! BTW...already tried updating and it broke. Hopefully, the fix won't be too hard to find.

rhett@dreamshot:~/oob/meteors/todos$ meteor
[[[[[ ~/oob/meteors/todos ]]]]]               

=> Started proxy.                             
=> Started MongoDB.                           
=> Errors prevented startup:                  

   While loading plugin `minifier-postcss` from package `juliancwirko:postcss`:
   packages/minifier-postcss/plugin/minify-css.js:33:8: Object [object Object] has no method 'existsSync'
   at meteorInstall.node_modules.meteor.minifier-postcss.plugin.minify-css.js (packages/minifier-postcss/plugin/minify-css.js:33:8)
   at fileEvaluate (packages/modules-runtime/.npm/package/node_modules/install/install.js:202:1)
   at require (packages/modules-runtime/.npm/package/node_modules/install/install.js:75:1)
   at <runJavaScript-75>:246:1
   at <runJavaScript-75>:252:3

I'm hoping that we can update the upstream eslint packages so we don't have to have too complicated a section in package.json. It should be simple to get a new project going with the same settings. If it's not, we should do something about it!

I don't think our starting point will grow much. As a user works with it though, I can see their rules section growing with project specific needs. In that case, it might be good for them to have it separated out. Also, for beginners, it might help to have reasons for the lines we've put in already (until we get those annotations in the guide 😄)

Just a thought I'm throwing out though. I'll move forward as is.

[rhettlivingston]

There's always something. I can't find a version of eslint-plugin-import that works for both NPM 3.9.6 and Node 0.10.45. So, I can't get ESLint working right with Meteor 1.3.4.1.

It confused me for a while because I can get a combination working if I install it with NPM 2.14.22 and then run it with NPM 3.9.6. So, I had it working before I updated the project (with NPM 2.14.22), updated to Meteor 1.3.4.1, it worked for a while, and then it broke when I did a fresh install of the NPM packages (with NPM 3.9.6).

I've reported the issue at import-js/eslint-plugin-import#415. If they can fix their latest stuff to work under Node < 4 with the older versions of ESLint, then we'd be good to go.

Note also that ESLint just released v3.0.0 on July 1st. It requires Node 4. Meteor 1.4 can't come soon enough.

[rhettlivingston]

12 days after I'd hoped,,, I believe that the updates contained in meteor/todos#163 address the concerns of this PR. This PR should be good to go.

Note that I have not changed anything in the guide to discuss the use of conditional imports and exports. This was a conscious choice because the guide is not 1.3.3 specific. In a future 1.3.3+ or 1.4 version, the existing language on using require where conditional imports may now be used should be changed.

[tmeasday]

Thanks again @rhettlivingston