WIP: Making relative paths absolute

[NickBusey]

I see a lot of relative paths throughout the codebase, like import { Todos } from '../../../api/todos/todos'; in todos-item.tests.js

It seems to me that import { Todos } from '/imports/api/todos/todos'; is far more readable and reliable. You can move the file around without breaking things. Is there a reason for all these relative paths as opposed to absolute? If not, I will go ahead and fix the rest of the paths I can find and finish this PR.

[NickBusey]

@hwillson @abernix Thoughts?

[hwillson]

Hi @NickBusey - there are pro's / con's to both approaches. Using the absolute approach can help with refactoring, but using relative paths is definitely more common in the larger npm ecosystem. Not to mention now that Meteor has better Babel / .babelrc support, we can also leverage Babel plugins like babel-root-import to make this even cleaner. That being said, relative paths were chosen by the original todos authors, and have stayed that way since. I'm not sure what the best path forward is here - I've used both approaches with success, so I'm on the fence. I do know though that changing this app to use absolute paths means all related code samples in the Guide would need to be updated as well.

@abernix @lorensr What do you guys think?

Just to add - this has been discussed in the forums (e.g. here and here), but there isn't really a clear preference per say ...

Thanks!

[lorensr]

IMO it's a nice feature of our build system that might be nice to highlight. I definitely think it's easier to use than calculating how many ..s to use, and you can move the file around without changing things. Seems also better in Guide snippets – you don't need to know the file-structure location of the snippet, and the import statement gives you get a clearer sense of the dir structure of the app.

[hwillson]

Thanks @lorensr! @NickBusey, if you're still up for it I think we're all systems go on finishing up this PR. We'll need to synch the Guide up as well, so if you're interested in submitting a PR including the related code changes over in the Guide repo, that would be awesome as well!

Then there's the question of updating the other active branches of todos, which are pretty much just the react and coffescript branches. I've been meaning to update the react branch with a few other changes, so once you're done with the Blaze version, I'll take care of it. Maybe we can tag @GeoffreyBooth to look into updating the coffeescript branch? Thanks again!

[NickBusey]

Haha @workflow thanks, I think we both squashed the commits at the same time. :)

[NickBusey]

I'm not sure what happened with the tests, looks like maybe my branch was out of date and the tests got moved in Master?

[NickBusey]

I'm going to try this again fresh..

[GeoffreyBooth]

Sorry to join late. What's there goal here? To only allow absolute paths? I don't think that's a good idea. For one thing, it breaks existing import spec AFAIK, not to mention backward compatibility. But more broadly, it's only better for small apps. Once your app is big enough to create modules in some form, and those modules themselves might have five or ten or twenty files, it becomes a hassle to maintain absolute paths to lots of files within that module. If you need to reach across the app with lots of ../..s, that's a sign you're probably doing something wrong.

I'd say if you're going to add this, make it optional.

[NickBusey]

@GeoffreyBooth No, nothing is changing in how meteor works or anything like that. This is just changing how the todos demo app imports are structured a bit to clarify things to people new to meteor, and make it slightly harder to break things when playing around with the 'todo' app. You can keep 'import'ing things however you like.

[GeoffreyBooth]

Maybe what makes more sense then is to use absolute paths when reaching outside of the current folder or module, and relative otherwise. There's a place for either.

[NickBusey]

I agree, and that's what my PR does. https://github.com/meteor/todos/pull/216

[benjamn]

Maybe what makes more sense then is to use absolute paths when reaching outside of the current folder or module, and relative otherwise. There's a place for either.

Strongly agree with this. Relative paths within the same project subdirectory tree make your code more portable, since you can move the whole directory somewhere else and all the imports inside of it continue to work.

Also, if you're tempted to refer to an external module by absolute path, perhaps it should be an npm package, so that you can refer to it by its package name, rather than bothering with relative or absolute identifiers. Not always appropriate, but at least there's a third option!

[abernix]

I don't think there is a definitively-right answer here. I personally use relative paths and my IDE automatically takes care of "counting" those dots for me. It could be argued that the relative import would save a pain point if you renamed an intermediate directory. Also, a blend of options is also reasonable, for example, an index.js that imports files "near" it (within a level, or two!) and perhaps an index.js being imported absolutely. Depending on the way your application is setup, your requirements may vary.

That being said, I think @lorensr's comment about it giving a clearer sense of the directory structure is very true, especially for the guide and I think I'm in favor of this because of that matter alone.

I think we should get a guide PR setup for this and merge simultaneously. @NickBusey would that be something you'd be able to help with?

[abernix]

Never mind my last comment – I had typed it before the last 8 comments and before I saw the new PR. It looks like this is going in the "best of both worlds" direction now, which is what I was getting at anyway.