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

Npm is undefined #244

Closed
s7dhansh opened this issue Mar 18, 2017 · 16 comments
Closed

Npm is undefined #244

s7dhansh opened this issue Mar 18, 2017 · 16 comments

Comments

@s7dhansh
Copy link

@s7dhansh s7dhansh commented Mar 18, 2017

Updating spacebars-compiler to 1.1.1 gives an error Npm is undefined while requiring uglify-js. Iassume it is because of 459f24f#diff-88dc7475eedf918122374be6d7c2c151L5.

Reverting to 1.1.0 works.

@mitar
Copy link
Collaborator

@mitar mitar commented Mar 18, 2017

True. @sethmurphy18, can you look into this. It seems compiler.js is included also on the client side:

And Npm.require is not available on the client side: https://github.com/meteor/blaze/blob/8558cb980428354417c282f6d011bb2a0e2adcd6/packages/spacebars-compiler/compiler.js

@mitar
Copy link
Collaborator

@mitar mitar commented Mar 18, 2017

@benjamn, @abernix, can we do Npm.require in package.js, and then import ... from instead of Npm.depends? Would that work? So that import would add it both on the client and server side.

And no idea why this was not cough before.

@s7dhansh: Are you using spacebars-compiler on the client side?

@s7dhansh
Copy link
Author

@s7dhansh s7dhansh commented Mar 19, 2017

@mitar i am not using it specifically. It seems to be used by static-html. I am not sure why it would use it on the client.

@eagerestwolf
Copy link
Contributor

@eagerestwolf eagerestwolf commented Mar 19, 2017

I am not 100% sure. I know some packages on Atmosphere use this. I could add a script on the server that requires then exports it.

@Dweez
Copy link

@Dweez Dweez commented Mar 19, 2017

Hello,
We could use spacebars-compiler on the client side to compile template strings stored in database. Some examples here http://stackoverflow.com/a/26923672

@mitar
Copy link
Collaborator

@mitar mitar commented Mar 19, 2017

Hm, an interesting question is why we ship spacebars compiler by default to the client? This I think is strange.

@yasaricli
Copy link

@yasaricli yasaricli commented Mar 20, 2017

Https://github.com/meteor/blaze/blob/8558cb980428354417c282f6d011bb2a0e2adcd6/packages/spacebars-compiler/package.js#L22

The file is being executed by both server and client.
The code on the client side gives an error because the (Npm.require) is on the server side.

UglifyJSMinify must be imported within the _beautify function.

@abernix
Copy link
Member

@abernix abernix commented Mar 21, 2017

The spacebars-compiler README.md states that it should not be loaded on the client. I'm not sure when we started deviating from that or when it changed, or if it's actually necessary on the client, but I guess we should make sure it works if some people are using it?

Did we ever get to the bottom of why we needed a beautify function at all? I found no comment or explanation or commit about the reasoning for it as far back in the Meteor repo history as I could go.

It should be possible to Npm.depends and then use require or import in the package, but that would require the inclusion of the modules or ecmascript packages within spacebars-compiler. I'd lean more toward figuring out if we can safely get rid of beautify.

@mitar
Copy link
Collaborator

@mitar mitar commented Mar 21, 2017

beautify: My understanding is that it makes the template code nicer. Template code is generated by combing JavaScript snippets together through code and it would look really ugly and unreadable.

What I worry is why would people get this error, if spacebars-compiler is not available on the client by default? Or are we providing it?

Anyway, can one import a Npm.depends package using ES6 import?

@abernix
Copy link
Member

@abernix abernix commented Mar 21, 2017

@mitar See my ninja-edit (last paragraph) above: Yes, I believe it should be possible to Npm.depends + require/import.

What I worry is why would people get this error, if spacebars-compiler is not available on the client by default? Or are we providing it?

This is a legit concern. If they are getting this error on the client, then it must be getting included on the client for some reason. I have not had a chance to investigate more.

@mitar
Copy link
Collaborator

@mitar mitar commented Mar 21, 2017

So I think the easiest and best approach is to not do beautification on the client side for now. I do not even know of use cases where people really compile blaze templates on the client side. We can see if they need this feature, we can think at that time what to do.

So I will just make it so that only server side does this.

The other two concerns I have, still unresolved, are:

  • why is spacebars compiler being loaded for people who did not explicitly want it on the client side (are we bundling whole compiler for people?)
  • why our tests didn't catch this issue
@mitar mitar closed this in 0aed367 Mar 21, 2017
@mitar
Copy link
Collaborator

@mitar mitar commented Mar 21, 2017

Released as 2.3.2.

@mokaid
Copy link

@mokaid mokaid commented Mar 22, 2017

spacebars-compiler@1.1.1 --> spacebars-compiler@1.1.0 , Solved it all ..
Hope this could help

@mitar
Copy link
Collaborator

@mitar mitar commented Mar 22, 2017

@mokaid: Try going to spacebars-compiler@1.1.2. That should also work. It does not for you?

@mokaid
Copy link

@mokaid mokaid commented Mar 22, 2017

@mitar worked perfectly ..
Thanks Amigo

@ssparacio
Copy link

@ssparacio ssparacio commented Mar 22, 2017

+1 on working.
Ran meteor update:

blaze upgraded from 2.3.1 to 2.3.2
caching-html-compiler upgraded from 1.1.1 to 1.1.2
spacebars upgraded from 1.0.14 to 1.0.15
spacebars-compiler upgraded from 1.1.0 to 1.1.2
templating upgraded from 1.3.1 to 1.3.2
templating-compiler upgraded from 1.3.1 to 1.3.2
templating-runtime upgraded from 1.3.1 to 1.3.2
templating-tools upgraded from 1.1.1 to 1.1.2
ui upgraded from 1.0.12 to 1.0.13

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

Successfully merging a pull request may close this issue.

8 participants
You can’t perform that action at this time.