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

Core: do not expose second argument of the jQuery.globalEval #2718

Closed
wants to merge 1 commit into from

Conversation

markelog
Copy link
Member

@mgol
Copy link
Member

mgol commented Nov 13, 2015

This should be a variable, i.e. it should lie in /core/var/.

@markelog
Copy link
Member Author

I don't think so @mzgol

@mgol
Copy link
Member

mgol commented Nov 13, 2015

Why not? A module that exports a single thing and doesn't have side effects should be a variable. Look at src/css/var/.

I see, though, that some other modules from core are not following the same principle, e.g. src/core/access.js should be a variable as well.

cc @timmywil

@markelog
Copy link
Member Author

Why not?

Didn't you answer your own question with -

I see, though, that some other modules from core are not following the same principle, e.g. src/core/access.js should be a variable as well.

There is a lot of modules like that. I don't think we should impose additional restrictions on the contributions process.

@mgol
Copy link
Member

mgol commented Nov 14, 2015

@markelog From what I remember this was not an arbitrary restriction, the concept of var-modules was created by @timmywil to save size in the built file. Refactoring your PR in this way saves 2 bytes.

I thought it was going to bring bigger gains, though... So, out of curiosity I applied it to other similar modules and... I got back to the size from this PR (i.e. I lost 2 bytes).

@timmywil, since those var-modules don't seem to save size as much as I thought (or not at all sometimes), could you clarify what's their purpose? Because, as @markelog pointed out, currently the decision whether to create a var-module or a regular one seems completely random looking at all the files.

@timmywil
Copy link
Member

The point of the var modules is to translate...

// File var/arr.js
define( function() {
    return [];
} );

to

var arr = [];

This is necessary whether the size gains are minor or not. For functions, though, the use of this translation is more subjective. I've mainly used var modules for smaller, one- to two-line, functions. I don't think we need to do that here.

@markelog
Copy link
Member Author

I know it might be sound... ahhh, weird :-), but should we document that?

@timmywil
Copy link
Member

should we document that?

Meh.

@timmywil
Copy link
Member

We could stick a note about it in CONTRIBUTING.md.

@markelog
Copy link
Member Author

Yeah, my thought exactly, not on contribute.jquery.com or anything like that

@markelog markelog closed this in 6680c1b Dec 2, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants