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

import tracker dep #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ejfrancis
Copy link

@ejfrancis ejfrancis commented Jul 20, 2017

(for #16)

  • import the Tracker dependency instead of relying on global

I've only tested this by adding the dependency in package.json with the file:// specifier, working fine for me.

@ejfrancis
Copy link
Author

@Akryum

@ejfrancis
Copy link
Author

ejfrancis commented Mar 30, 2018

@Akryum can you please merge this and publish a release?

@Akryum
Copy link
Collaborator

Akryum commented May 13, 2018

This doesn't work for me.

Error: Cannot find module 'meteor/tracker'

@ejfrancis
Copy link
Author

@Akryum that's strange, it's right from the official docs and works for me

@mitar
Copy link
Member

mitar commented May 13, 2018

This is a NPM package, not Meteor package. So I am not sure this applies.

@ejfrancis
Copy link
Author

ejfrancis commented May 13, 2018

@mitar It's an npm package but it will only ever be used in the context of a Meteor application so the meteor/ imports should always be accessible, no? I've been pointing to my fork in a Meteor app for months and it works fine

This shouldn't have any functional difference, it just makes it play nicely with other tools like Jest which make some assumptions about your code explicitly declaring imports

@Akryum
Copy link
Collaborator

Akryum commented May 14, 2018

Strange. I just tried it in my Meteor app and it throws the error. Maybe there is an issue with Meteor not processing imports in npm packages the same way it does in app/atmosphere code?

@mitar
Copy link
Member

mitar commented May 14, 2018

It's an npm package but it will only ever be used in the context of a Meteor application so the meteor/ imports should always be accessible, no?

meteor/ might be accessible, true, but meteor/tracker should be only accessible if parent application uses it and has it as a dependency. You do not declare dependency anywhere, so you cannot assume it is there. And you cannot declare it from NPM package. I suggest that if this is really meant to be used only with Meteor, it is made into Meteor package.

@Akryum
Copy link
Collaborator

Akryum commented May 14, 2018

In Jest I think this will work:

beforeEach(() => {
  window.Tracker = Tracker
})

@ejfrancis
Copy link
Author

I'm not sure of how the meteor build system differs between its treatment of npm packages and atmosphere packages

This is what I'm using in my package.json right now

    "vue-meteor-tracker": "ejfrancis/vue-meteor-tracker#import-tracker-checkin-build",

then in my client index I'm just importing it as usual and it works

import VueMeteorTracker from 'vue-meteor-tracker';

I'm on Meteor 1.6.1.1

@mitar the official Meteor docs still state that some day all packages will be moved to npm, so I think we should be trying to keep packages on npm if possible but you are right that they do state that one of the scenarios when you should use atmosphere package is if Depend on core Meteor packages, such as ddp and blaze, and for now Tracker is still on atmosphere. So technically I suppose this does belong on atmosphere as far as MDG is concerned (for now)

@Akryum I believe you're right about the beforeEach() usage

@mitar
Copy link
Member

mitar commented May 14, 2018

@mitar the official Meteor docs still state that some day all packages will be moved to npm

I do not think this will really happen any time soon, we are waiting for two years here on this: meteor/blaze#13

So i would not bet things on this. In any case, until they are on NPM, you cannot depend on them.

@ejfrancis
Copy link
Author

It's true that an npm package can't declare a dependency on a Meteor package at a package-level, so this package is in a bit of a strange state because it does explicitly depend on Tracker being available but nowhere in the code is this dependency declared. At least it's possible to declare the dependency on a code-level though via import, which I believe would be better than nothing

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

Successfully merging this pull request may close these issues.

3 participants