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.

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

Needs to handle meteor/ packages as well #1

Closed
trajano opened this issue May 1, 2016 · 11 comments
Closed

Needs to handle meteor/ packages as well #1

trajano opened this issue May 1, 2016 · 11 comments

Comments

@trajano
Copy link

trajano commented May 1, 2016

This solves the /imports/ folder nicely, but meteor/ packages need to be handled as well. From what I can tell there are two options.

  1. .meteor/packages
  2. .meteor/versions

I would opt for the following logic. If import has : check .meteor/packages (since it is something a user would've added, else check .meteor/versions so we do not have to meteor add the many implicit packages such as meteor, mongo, etc.

trajano added a commit to trajano/meteor-boilerplate that referenced this issue May 1, 2016
Used airbnb and meteor recommended eslint

Removed '/imports/' ignore

The custom resolver allows the use of '/' for the linter now.  So it works
with more than just '/imports'

Still kept 'meteor/` ignore until
clayne11/eslint-import-resolver-meteor#1 gets
resolved.
trajano added a commit to trajano/meteor-boilerplate that referenced this issue May 2, 2016
Used airbnb and meteor recommended eslint

Removed '/imports/' ignore

The custom resolver allows the use of '/' for the linter now.  So it works
with more than just '/imports'

Still kept 'meteor/` ignore until
clayne11/eslint-import-resolver-meteor#1 gets
resolved.
@clayne11
Copy link
Owner

clayne11 commented May 2, 2016

It's not so simple as just resolving that a certain package exists. The job of the resolver is to find the file that exports the symbols that an import is looking for. Since Meteor packages (including the internal Meteor packages such as Tracker, Email, etc) don't use Npm, they don't necessarily have a file that exports the correct symbols.

Additionally, there's no central location to look for the mainModule file (which as I described might not actually have proper exports defined). Meteor packages are downloaded and cached in ~/.meteor, and when they are built into a project the reside in <PROJECT>/.meteor/local/isopacks.

Unfortunately, neither of these locations exist before a project is built for the first time, meaning we would have issues trying to run the resolver in a CI build process, and it wouldn't be reliable anyways since many Meteor packages, both internal and from Atmosphere, don't take advantage of ES6 modules.

I don't see a good way of solving this issue, but if you have any suggestions I'm happy to hear them and try to come up with a solution for this.

@trajano
Copy link
Author

trajano commented May 2, 2016

I am not sure about the exports at least I didn't see it in the resolver spec. It states only needs to return found and path. It also documents that path can be null so we can use it for Meteor modules like that as long as it is in .meteor/versions

trajano added a commit to trajano/meteor-boilerplate that referenced this issue May 2, 2016
Used airbnb and meteor recommended eslint

Removed '/imports/' ignore

The custom resolver allows the use of '/' for the linter now.  So it works
with more than just '/imports'

Still kept 'meteor/` ignore until
clayne11/eslint-import-resolver-meteor#1 gets
resolved.
@clayne11
Copy link
Owner

clayne11 commented May 4, 2016

I think you're right. We can resolve that a file was found, but we will have to add ^meteor/ to import/ignore because if won't be able to resolve the symbols. I'll have a look at this when I get a chance, probably this weekend. You're welcome to try a PR if you'd like.

@trajano
Copy link
Author

trajano commented May 4, 2016

Pass on the PR for now. RL taking effect :)

@zeroasterisk
Copy link

thanks for this, I needed it --- I too would like the meteor paths, but will happily settle for ignoring them for now...

might be worth a paragraph on the README w/ a "how to ignore meteor imports"

@clayne11
Copy link
Owner

clayne11 commented May 5, 2016

Good idea - I'll do one better. I'll set it to automatically ignore them so no config is required.

#2

@clayne11
Copy link
Owner

clayne11 commented May 5, 2016

I took the second option of looking in .meteor/versions since a package in .meteor/packages can imply other packages.

Essentially using .meteor/packages could lead to false negatives and .meteor/versions can lead to false positives. I chose the latter.

Which do you guys think is a better option, false negatives or false positives?

This is published under 0.1.0.

@clayne11 clayne11 reopened this May 5, 2016
@trajano
Copy link
Author

trajano commented May 5, 2016

I prefer the the logic I wrote in my OP where it will only do .meteor/version for those that have no : and .meteor/package for others. This enforces that if we are importing something on a specific package that we are adding the package.

@clayne11
Copy link
Owner

clayne11 commented May 5, 2016

Oh I see - I mis-read your original post. I thought you were suggesting using .meteor/versions as a fallback, which didn't make sense to me since .meteor/packages is a subset of .meteor/versions.

That makes sense though. I will change the logic to do that.

@clayne11
Copy link
Owner

clayne11 commented May 8, 2016

OK, I have made the change in v0.2.0 to the logic you laid out. I also updated the README to make the installation instructions clearer.

@trajano
Copy link
Author

trajano commented May 9, 2016

Confirmed fix on https://github.com/trajano/meteor-boilerplate/tree/meteor-skeleton-1.0.3 it even found one I missed myself :)

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

No branches or pull requests

3 participants