Skip to content

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.

4 participants