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.

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

code style changes for skel-full #8045

Merged
merged 5 commits into from Nov 17, 2016

Conversation

Projects
None yet
6 participants
@dburles
Contributor

dburles commented Nov 11, 2016

Updated the code style (and dependencies) for the full scaffold to match (as best as seems reasonable) the Meteor guide.

There is some discrepancy with the recommended eslint configuration. For example there are a number of rules that make sense within the context of a Meteor project, which is exactly why these have been added in the todos example. So to address that I've added a few eslint-disable comments where it makes sense, though it's not ideal.

In some cases like this it is pretty much standard practise (in Meteor) to not bother with naming function expressions (func-names). So we should probably change that, but as you can see it's not great that it then conflicts with the recommended (stock) linter config.

Perhaps a solution is that it can be handled by the eslint meteor plugin, though I'm not totally sure how that might work since it's designed to work independently to the airbnb rules. @dferber90 any thoughts? :)

So in the meantime until there's a solution like above should we remove the eslint-disable comments for the sake of simplicity?

@abernix

Overall some great changes here!

As to the ESlint stuff, I'm really not up to date on the latest in eslint suggestions for Meteor. I'd agree it's a bit funky, but will leave that to others to recommend.

@@ -5,6 +5,7 @@
"start": "meteor run"
},
"dependencies": {
"meteor-node-stubs": "~0.2.0"
"babel-runtime": "^6.18.0",

This comment has been minimized.

@abernix

abernix Nov 11, 2016

Member

I can see your concern about the dependencies here, but keeping the actual latest version number here isn't critical.

The most recent compatible version of the packages will still be installed anyhow (thanks to semver) when the user runs meteor npm install.

And for those who don't run meteor npm install, the version listed in their own (newly created) package.json will get updated soon after they've created their app since users will be given the warning to meteor npm install babel-runtime --save. Running that will bump this version, and install the latest simultaneously.

They can probably just be updated when someone notices.

This comment has been minimized.

@dburles

dburles Nov 11, 2016

Contributor

Yeah I thought the same, though if it was fairly trivial to implement an automated method I figure it would be worthwhile

export const Links = new Mongo.Collection('links');
const Links = new Mongo.Collection('links');
export { Links };

This comment has been minimized.

@abernix

abernix Nov 11, 2016

Member

Is there a particular reason this is the preferred syntax over the prior?

This comment has been minimized.

@dburles

dburles Nov 11, 2016

Contributor

My thinking is:

  1. It indicates that it will be likely that you'll export more from this file (validated-methods, etc)
  2. I think it's a good convention to place all exports at the base of each file, otherwise it can be difficult to determine what's exported

This comment has been minimized.

@stubailo

stubailo Nov 11, 2016

Contributor

I think it's a good convention to place all exports at the base of each file

I kinda disagree, this seems like a throwback to module.exports = { ... }. You can easily see what is exported just by searching for the export keyword.

This comment has been minimized.

@dburles

dburles Nov 11, 2016

Contributor

What's the disadvantage though?

This comment has been minimized.

@stubailo

stubailo Nov 11, 2016

Contributor

Writing the exported variable twice? It just seems unnecessary. If there are no advantages then the most concise format wins IMO

This comment has been minimized.

@dburles

dburles Nov 11, 2016

Contributor

Ok I've gone with the more concise version

if (!url.match(/((http|https)\:\/\/)+[a-zA-Z0-9\.\/\?\:@\-_=#]+\.([a-zA-Z0-9\&\.\/\?\:@\-_=#])*/g)){
throw new Meteor.Error('Bad url. I.e https://www.meteor.com');
const re = /((http|https)\:\/\/)+[a-zA-Z0-9\.\/\?\:@\-_=#]+\.([a-zA-Z0-9\&\.\/\?\:@\-_=#])*/g;
if (!url.match(re)) {

This comment has been minimized.

@abernix

abernix Nov 11, 2016

Member

RegExp.prototype.test() has long been proven to be more performant than String.prototype.match() in the browser. These days, I find them to be the same in the Node environment, but for the sake of consistency I'd recommend changing this to:

if (!re.test(url)) {

Meteor.methods({
'links.insert'(title, url) {
check(url, String);
check(title, String);
// Check if this is a valid url
if (!url.match(/((http|https)\:\/\/)+[a-zA-Z0-9\.\/\?\:@\-_=#]+\.([a-zA-Z0-9\&\.\/\?\:@\-_=#])*/g)){
throw new Meteor.Error('Bad url. I.e https://www.meteor.com');
const re = /((http|https)\:\/\/)+[a-zA-Z0-9\.\/\?\:@\-_=#]+\.([a-zA-Z0-9\&\.\/\?\:@\-_=#])*/g;

This comment has been minimized.

@abernix

abernix Nov 11, 2016

Member

In general, I dislike having such a subjective RegEx in this sample and I'd almost propose changing the entire method to something else (e.g. colors.insert shrug) based purely for that reason.

As to the RegEx itself, it doesn't need the /g modifier and lacks any anchors (e.g. ^, $). Honestly, I hope this isn't used to actually verify a URL anywhere. For example, it would match:

hey sup regex http://http://http://?. <script><!-- // (c) 1997 //--></script>

...as a valid URL, amongst many other things.

I propose we get rid of this in any way possible.

This comment has been minimized.

@stubailo

stubailo Nov 11, 2016

Contributor

Yeah, I'd suggest just getting rid of RegEx entirely.

This comment has been minimized.

@dburles

dburles Nov 11, 2016

Contributor

It's hard to know where to draw the line as far as implementing more correct validation, I think it would just be too much for the purpose of this scaffold. I'd be ok with not validating.

This comment has been minimized.

@stubailo

stubailo Nov 11, 2016

Contributor

Honestly validating URLs doesn't seem like a good idea in general. Perhaps you could just check if it starts with http or something.

This comment has been minimized.

@dburles

dburles Nov 11, 2016

Contributor

I agree, I had considered removing it as part of these updates

dburles added some commits Nov 11, 2016

Template.info.onCreated(function infoOnCreated() {
/* eslint-disable func-names, prefer-arrow-callback */

This comment has been minimized.

@abernix

abernix Nov 16, 2016

Member

I find it slightly weird to have this (or any of these) hovering mid-file. Perhaps hoist all of these to the top?

But on that note, maybe the eslint stuff should just be removed until it's figured out what the best way to do it is? With having these eslint-disable lines here (And thus implying an eslint "setup" is in place), it almost seems like setting up the new package.json with an eslintConfig and incorporate eslint-plugin-meteor, etc. might be better.

Anyhow, everything else looks great.

This comment has been minimized.

@dburles

dburles Nov 16, 2016

Contributor

Yep will remove, the eslint config already setup would be ideal for sure, let's go with that.

@dferber90

This comment has been minimized.

Contributor

dferber90 commented Nov 16, 2016

@dburles The way the ESLint ecosystem works is that plugins like eslint-plugin-meteor provide additional rules to be used (they can also export default configurations, but it's more common to have separate config packages). Configs like eslint-config-airbnb specify which plugins to use (if any), how they should be configured, and they can also configure the core rules. What you wanna do is create eslint-config-meteor and specify the rules there. It can probably use eslint-config-airbnb behind the scenes and it can also provide some defaults for eslint-plugin-meteor. See Shareable Configs for how to create one. If you want to chat about this face to face just let me know :) Happy to help

@dburles

This comment has been minimized.

Contributor

dburles commented Nov 16, 2016

Nice! thanks @dferber90. I had wondered if it were possible to handle it like that but hadn't looked into it, sounds great. I'll get that setup and let you know once I've got something we can start with :)

@iDoMeteor

This comment has been minimized.

iDoMeteor commented Nov 16, 2016

One thing to note, is that the linting MDG uses for Meteor is utilized in a considerably different context than that of an app built with Meteor. :)

@dburles

This comment has been minimized.

Contributor

dburles commented Nov 16, 2016

Yeah, just to be clear it's not the Meteor core lint config, it's a lint config built for projects

@abernix

This comment has been minimized.

Member

abernix commented Nov 16, 2016

@dburles Do you think you can nuke those eslint changes (for now) and then revisit the more proper config in a new PR? It may be too late to float this, but it would be nice to have these changes in the 1.4.2.2 release (where it will make its first official appearance).

@dburles

This comment has been minimized.

Contributor

dburles commented Nov 16, 2016

@abernix Yeah definitely, in that case I would really like to get the other ones done too, what's the ETA on the release?

@dburles

This comment has been minimized.

Contributor

dburles commented Nov 16, 2016

@abernix actually nevermind, they're all good!

@abernix abernix referenced this pull request Nov 16, 2016

Merged

Release 1.4.2.2 (and 1.4.2.3) #8044

5 of 8 tasks complete
@abernix

LGTM. Thanks, @dburles.

It's discussed in #8044 that it will be soon. Not sure if this will make it but I've proposed it.

@benjamn

This comment has been minimized.

Member

benjamn commented Nov 16, 2016

Merged into #8044, thanks!

@benjamn benjamn closed this Nov 16, 2016

@benjamn benjamn reopened this Nov 17, 2016

@benjamn benjamn merged commit 00c6681 into meteor:devel Nov 17, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
@dburles

This comment has been minimized.

Contributor

dburles commented Nov 18, 2016

@abernix @dferber90 @iDoMeteor here's a start, it's not yet published to npm. https://github.com/dburles/eslint-config-meteor ideas/feedback are welcome. The idea is that this will be the official config referenced within the guide. Which then means we can be confident that if this configuration is added to skel-full there will be no linting errors to speak of :)

@dferber90

This comment has been minimized.

Contributor

dferber90 commented Nov 18, 2016

@dburles Did a quick overview and this looks good 👍

You may want to introduce upper limits to your peer dependencies since new rules introduced into them through major releases can mean that the build of people using eslint-config-meteor suddenly fails linting without them doing a major upgrade of anything themselves. I created meteor/eslint-config-meteor#1 to track this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment