veliovgroup / flow-router

🚦 Carefully extended flow-router for Meteor
https://packosphere.com/ostrio/flow-router-extra
BSD 3-Clause "New" or "Revised" License
202 stars 29 forks source link

weird bug with `import { Template } from 'meteor/templating'` #43

Closed hacknlove closed 6 years ago

hacknlove commented 6 years ago

import { Template } from 'meteor/templating' stop working since last meteor update

I had a working meteor application.

I updated to meteor 1.6 and then Template was undefined wherever js file with import { Template } from 'meteor/templating'

If I remove import { Template } from 'meteor/templating' everything runs fine with Template from global space.

If I remove ostrio:flow-router-extra I can import { Template } from 'meteor/templating' again.

The next meteor app reproduces the bug templating.tar.gz

mikhail-shishov commented 6 years ago

Hello @hacknlove ,

Which version of flow-router-extra do you use? Also, have you installed templating package as it was advised in this comment?

//cc @dr-dimitru

coagmano commented 6 years ago

I had the same issue, removing ostrio:flow-router-extra fixes the error (but makes app unusable)

Tried adding, removing & adding templating and blaze-html-templates with no effect

Reinstalling to get the version number (3.4.5) seems to have fixed it 🤷‍♂️

coagmano commented 6 years ago

I have a suspicion that dropping the dependency on templating and blaze confused Meteor's bundler, so I've added a PR #44 to add them back in as weak dependencies.

dr-dimitru commented 6 years ago

Hmm, I'll require minimal reproduction, as I've tested it before publishing, and now it works well too.

Btw weak dependency looks fine too.

Could you please check Templating package placed above flow-router-extra in .meteor/packages file??

coagmano commented 6 years ago

Just tested with @hacknlove's repro and it has the error, which is fixed by including templating above ostrio:flow-router-extra in packages and also fixed by adding the weak dependencies.

Which confirms my suspicions of Meteor not making templating available to flow-router.

In addition, using require to test if templating is available seems to cause require to "forget" what templating is...

I then tried replacing the require + try/catch:

try {
  Blaze    = require('meteor/blaze').Blaze;
  Template = require('meteor/templating').Template;
} catch (e) {
  // we're good
}

with:

if (Package['templating'] && Package['blaze']) {
  Blaze    = Package['blaze'].Blaze;
  Template = Package['templating'].Template;
}

Which prevents require from "forgetting" templating. (and is the way mentioned quietly in the docs: https://docs.meteor.com/api/packagejs.html#PackageAPI-use)

I've added the change from require to Package in the PR as well

dr-dimitru commented 6 years ago

Reopened until new release is published

dr-dimitru commented 6 years ago

@coagmano I didn't get this one:

In addition, using require to test if templating is available seems to cause require to "forget" what templating is...

How require makes templating be forgotten?

coagmano commented 6 years ago

I have no idea, it seems to be a bug in require's implementation 🤷‍♂️ Don't know if it's meteor's implementation or part of commonjs

Because really, when require throws an error for a package that isn't available to flow-router-extra, it shouldn't then make that package not available to anything else ever...

dr-dimitru commented 6 years ago

@coagmano thank you for update, this what I was looking for:

make that package not available to anything else ever

dr-dimitru commented 6 years ago

@coagmano thank you for the fix. @hacknlove thank you for rising this question up. Please update to the latest release, this issue should be solved now - v3.4.6