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 to specify an objectToExportTo for goog.exportSymbol #2082

Merged
merged 4 commits into from May 21, 2014

Conversation

ahocevar
Copy link
Member

This allows users to build ol3 without anything exposed in the global namespace. This can e.g. be useful for creating an ol3 AMD module, by simply using a build configuration that contains

{
  "objectToExportTo": "o",
  "compile": {
    "output_wrapper": "define('ol',function(){var o={};%output%return o.ol;});"
  }
}

@tschaub
Copy link
Member

tschaub commented May 17, 2014

I like the sound of this. I think I'd call it namespace instead of objectToExportTo. Looks like there is a stray debugger in there. I'd like to give this a proper review but am not in a place to do so now.

@ahocevar
Copy link
Member Author

I removed the stray debugger, thanks for the catch. The objectToExportTo name is used as argument name in goog.exportSymbol. I think namespace might be confusing, because the default is goog.global. But no strong opinion on this, so happy to change if it's just me thinking so.

@tschaub
Copy link
Member

tschaub commented May 20, 2014

@ahocevar it looks like 0d21d45 is missing the changes to generate-exports.js.

Here's my thinking on using namespace over objectToExportTo:

  • all other members in the build config are snake_case
  • people (JavaScript people at least) often use the term "namespace" in saying things like the "global namespace" or "avoid namespace pollution"
  • people will be reading our docs instead of the Closure Library source when using the build tool

Basically, I just think the name objectToExportTo is a bit ugly. I'd go for namespace or context instead.

@bartvde
Copy link
Member

bartvde commented May 20, 2014

I agree with @tschaub on the naming here

@ahocevar
Copy link
Member Author

Ok, you got me convinced. namespace it shall be. I'm updating the pull request now, and also see where the generate-exports.js changes got lost.

This allows users to build ol3 without anything exposed in the
global namespace. This can e.g. be useful for creating an ol3
AMD module, by simply using a build configuration with
"define('ol',function(){var o={};%output%return o.ol;});" as
"output_wrapper".
@ahocevar
Copy link
Member Author

I updated the pull request now, and also added the changes to generate-exports.js. I'll add another commit which mentions the new config option in the readme.

@ahocevar
Copy link
Member Author

Readme updated. This is now ready for a final review.

@tschaub
Copy link
Member

tschaub commented May 21, 2014

Looks great @ahocevar. Here are my comments in form of commits: https://github.com/tschaub/ol3/compare/ahocevar:custom-exports-target...custom-exports-target

I updated the namespace example to use a namespace less likely to collide with a name already used by the compiler. In a full build that I just ran, the compiler used 29 single letter names in the global namespace, so ~55% chance of collision with a single letter namespace (meaning the compiler would redefine the namespace provided in the wrapper - this is only a rough probability because it doesn't look like the compiler uses $ uniformly). The same build has 1187 two letter names (roughly 43% chance of collision) and zero three letter names.

Anyway, this is a nice enhancement.

People should be discouraged from exporting to single letter namespaces to avoid collissions with the symbols generated by the compiler.
@ahocevar
Copy link
Member Author

Thanks for the improvements @tschaub!

ahocevar added a commit that referenced this pull request May 21, 2014
Allow to specify an objectToExportTo for goog.exportSymbol
@ahocevar ahocevar merged commit cabd436 into openlayers:master May 21, 2014
@ahocevar ahocevar deleted the custom-exports-target branch May 21, 2014 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants