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

allow bare javascript files on the server #3681

Closed
mitar opened this issue Feb 8, 2015 · 25 comments
Closed

allow bare javascript files on the server #3681

mitar opened this issue Feb 8, 2015 · 25 comments

Comments

@mitar
Copy link
Contributor

@mitar mitar commented Feb 8, 2015

I know there was a lot of discussion about this already done, but I find this really the worst part of Meteor. Having to modify upstream code so that one can use a global symbol defined in a library is really painful. Meteor tries to be smart and differs between, at the top level:

var FileScope = {};

And:

PackageScope = {};

The first becomes file scoped, while the second package scoped (and potentially exported). I think this is an interesting approach, but it fails for 3rd party libraries, because for them both syntaxes are the same and when they are defining global (browser) scope, they almost always use the first scope. I am suggesting a simple extension to current packaging:

api.addFiles([
  'dist/library.js'
], 'client', {packageScope: ['FileScope']});

This would make variable FileScope behave like package scope, no matter if it was defined var FileScope. This would address 99% of issues I had encountered while trying to package 3rd party libraries.

@mitar
Copy link
Contributor Author

@mitar mitar commented Feb 8, 2015

(The other common use case is for libraries expecting exports, but let's leave this to some other time. So client side NPM modules.)

@gadicc
Copy link
Contributor

@gadicc gadicc commented Feb 9, 2015

I've also been thinking about "wrapping options" to be specified in package.js. My idea was a bit different but I think would also be really helpful for wrapping 3rd party packages. What if we just had:

api.addFiles([
  'dist/library.js'
], 'client', { wrapper: 'AMD' });

etc. The idea is actually that these "wrapper modules" could potentially be 3rd party Atmosphere packages, supporting different methods. Generally it's just a matter of setting up the appropriate methods and objects (say require, define, modules, exports) beforehand in the right scope, and then processing/exporting afterwards. If no wrapper is specified, the default Meteor policy is used, and either way, extra options could be specified (like the PackageScope stuff you suggest), i.e. the two ideas are not mutually exclusive but rather complementary.

This is also very relevant for ES6 / Harmony code, which currently is a total pain to get into Meteor.

@mitar
Copy link
Contributor Author

@mitar mitar commented Feb 9, 2015

Good ideas.

@glasser
Copy link
Member

@glasser glasser commented Feb 10, 2015

Apparently it's not documented (I thought it was at some point) but isn't this what the { bare: true } option to api.addFiles does? (And the client/compatibility directory in an app, which is documented.)

@glasser
Copy link
Member

@glasser glasser commented Feb 10, 2015

(Focusing on the original issue and not the followup comments here.)

@mitar
Copy link
Contributor Author

@mitar mitar commented Feb 11, 2015

But I would like to package things correctly. Yes, we can be putting everything with bare, but that then pollutes the global namespace. NPM and all other package systems already do that. Meteor should provide a way to piggy-back on top of them. And my proposal or @gadicc's proposal are trying to address that.

@glasser
Copy link
Member

@glasser glasser commented Feb 11, 2015

Oh, I see. You want to be able to do this at the per-symbol level?

It does seem a little surprising to me to assert that library authors who write var FileScope = {} intending that variable to be useful outside of the file will write other vars in the same file that they don't intend to be exported, though. Do you have examples?

BTW, what bare does is "don't wrap this file in a closure". In the app, that means that top level vars end up as globals, but in packages it just means they end up as package variables --- it doesn't affect the package-level closure. (Apps don't have a package-level closure, mostly to make use of the JS console easier.)

Right now most of what linker does is add code to existing scripts. This would also require finding and deleting the var. Not impossible (we do it with coffeescript a bit) but definitely a little different.

@mitar
Copy link
Contributor Author

@mitar mitar commented Feb 11, 2015

I am saying that library authors writing var FileScope at the top level of the file expect it to be a global variable. To my knowledge, writing var FileScope and FileScope is exactly the same in JavaScript in browsers at the top level of a file? So there is nothing strange for them to do that, expecting that it will be included in a normal JavaScript environment, thus becoming global. Of course for Meteor we do not want things to be global, so we have to find a way to make extract them out.

Here is example. In normal use, Handsontable is a globally available symbol. If I would add it with bare, it would be available. But I do not want to use bare. That defeats the whole purpose of Meteor packaging and namespacing system.

BTW, what bare does is "don't wrap this file in a closure". In the app, that means that top level vars end up as globals, but in packages it just means they end up as package variables --- it doesn't affect the package-level closure.

Oh. This is a little known fact. Interesting. So I just user bare and then export the symbol and I am all set? It will not leak out to the global scope automatically?

Right now most of what linker does is add code to existing scripts. This would also require finding and deleting the var. Not impossible (we do it with coffeescript a bit) but definitely a little different.

Aha. I thought you are already doing this for JavaScript as well.

@glasser
Copy link
Member

@glasser glasser commented Feb 11, 2015

OK, does this additional fact about bare satisfy your original needs here? (Though bare should be documented.)

@onip
Copy link

@onip onip commented Feb 19, 2015

just confirming that api.addFiles ('foo.js', 'client', {bare: true}); makes foo.js global variables ( var Foo = ... ) available at package-level closure so that they could be then exported via api.export ('Foo', 'client');. Other library variables won't pollute app global environment, unless exported of course.

This feature should definetely be documented on docs.meteor.com

@mitar
Copy link
Contributor Author

@mitar mitar commented Feb 26, 2015

I can confirm it works.

@mitar mitar closed this Feb 26, 2015
@mitar
Copy link
Contributor Author

@mitar mitar commented Feb 26, 2015

I am reopening this because it bare does not work on the server side. How can I make a library which was made for a client with var FileScope symbol defined to work on the server? There is no strange things, it simply declares the symbol, no window stuff and other stuff.

@mitar mitar reopened this Feb 26, 2015
@mitar
Copy link
Contributor Author

@mitar mitar commented Feb 26, 2015

It is exactly the same problem. Because it uses var FileScope the symbol is not available. If I modify it to PackageScope, it works.

@avital
Copy link
Contributor

@avital avital commented Feb 26, 2015

(Not necessarily to disagree, but this is the same problem that Node code
has in general -- if you copy a client library that uses top-level vars
expecting them to be globals into a node file and require it it won't
work)

On Thu, Feb 26, 2015 at 11:12 AM, Mitar notifications@github.com wrote:

Reopened #3681 #3681.


Reply to this email directly or view it on GitHub
#3681 (comment).

@mitar
Copy link
Contributor Author

@mitar mitar commented Feb 26, 2015

Yes, but in Node I have then some toolchains I can use to make it work. :-) Is there a known workaround which does not require changing upstream code?

@glasser
Copy link
Member

@glasser glasser commented Mar 3, 2015

What do you mean by "bare does not work on the server side"?

@mitar
Copy link
Contributor Author

@mitar mitar commented Mar 3, 2015

You cannot use it. It throws an error saying that bare is available only on the client side.

@mitar
Copy link
Contributor Author

@mitar mitar commented Mar 3, 2015

So api.addFiles ('foo.js', 'server', {bare: true}); throws an error.

@glasser glasser changed the title Easier adding of existing libraries to packages allow bare javascript files on the server Mar 28, 2015
@glasser
Copy link
Member

@glasser glasser commented Mar 28, 2015

Hmm, I forgot that. You're right. Looks like that goes back to the very first implementation of bare (in its initial name of compatibility): fb61e88#diff-2588798e782de3a0b93513cc522d73faR357

But that was back before linker. I don't see any reason you shouldn't be able to bare-ify a server file as long as the whole package still gets closured. I think we basically had this restriction because we didn't think there was a use case for it. I think the argument basically was that it was common to find client code written assuming var X = would be global, but rare for server code (as @avital said).

If our assumptions were wrong and there is a use case, I think the restriction could just be dropped...

@mitar
Copy link
Contributor Author

@mitar mitar commented Mar 28, 2015

Yes, please. This would really help packaging some libraries without having to modify them. The main use case is to package code made for client side to work also on the server side. As you are saying, client side code often has var X =. But if you want to use it on the server, then bare is useful.

@avital
Copy link
Contributor

@avital avital commented Mar 28, 2015

I'm curious @mitar -- what do people in vanilla Node do in this case? I think those libraries wouldn't be made to work in that case? Or do those libraries maybe check explicitly to see if require is defined and work a different way?

@mitar
Copy link
Contributor Author

@mitar mitar commented Mar 28, 2015

Yes, this is really to wrap some libraries targeting browsers. Not sure what they do, probably use grunt to modify the script during the build process.

@glasser glasser closed this in 0371c4f Apr 1, 2015
@glasser
Copy link
Member

@glasser glasser commented Apr 1, 2015

OK, that's convincing, and it's an easy enough change. Thanks!

@mitar
Copy link
Contributor Author

@mitar mitar commented Apr 3, 2015

Great!

@trusktr
Copy link
Contributor

@trusktr trusktr commented Jul 15, 2015

I landed here due to the 'bare' option may only be used for web targets error.

Besides this issue, I'm almost done making the first usable version of rocket:module (basic info is here for now), which aims to provide one solution to this problem of using 3rd party libraries in Meteor, plus it adds suport for CJS/AMD/ES6 modules (using Webpack behind the scenes) so you can use normal require/module.exports/define/import/export in your files. I'll update the README with usage instructions and documentation for the first release (1.0.0) very very soon. (don't use the current release of rocket:module, it won't do anything, I was testing Atmosphere publishing.)

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.

None yet
6 participants