Skip to content

Conversation

sheppard
Copy link
Contributor

Thanks for adding AMD support to Leaflet! At some point, I'd like to follow up on the discussion in #1364 about using AMD for Leaflet's internal modules as well - but for now I have a simple request: can you change the module declaration to be anonymous? Explicitly declaring the module name leads to inflexibility, as I'll explain.

Per the RequireJS docs, modules should generally be declared anonymously in order to support full portability between projects. For example, if I want to put leaflet in a lib/ subfolder in my JS directory, the resolved module name might be lib/leaflet (or wq/lib/leaflet), not leaflet. By defining the module anonymously, projects can integrate Leaflet wherever they wish without needing to define a RequireJS "paths" configuration pointing any dependencies on leaflet to their project's location of the file.

One of the only libraries that declares its AMD module explicitly is jquery, and this is largely for historical reasons - with the idea that this special case for jquery will be removed over time.

You'll notice that in this PR I've also removed the function that returns L - this is not needed since define() can be used directly with simple objects, in which case it will just return the object.

Edit 2013-11-18: Note that AMD best practice is to put third party libs at the AMD baseUrl so that they can be referenced by their common names. As above, the purpose of this is to avoid needing a complicated paths config for RequireJS or a similar loader. This is especially important when integrating a plugin such as Proj4leaflet, which AMD-depends on both 'proj4' and 'leaflet'. In general, Leaflet will likely be referenced from other AMD modules as 'leaflet' (and not './leaflet' or 'lib/leaflet'). So, my initial motivation for making Leaflet's AMD definition anonymous was not fully correct (and I am updating my library accordingly). Nevertheless, it is still recommended to use anonymous module definitions in nearly all cases.

@mourner
Copy link
Member

mourner commented Jun 24, 2013

Looks good, I agree. Thanks!

mourner added a commit that referenced this pull request Jun 24, 2013
make AMD declaration anonymous
@mourner mourner merged commit 01b76b7 into Leaflet:master Jun 24, 2013
@Manumie
Copy link

Manumie commented Oct 15, 2013

Hello, sorry for unearthing this issue and the probably dumb question coming along.
I'm trying to use Leaflet with Ember App Kit (AMD modules built with almond.js) , but I get this error on loading the app :

Cannot read property splice of undefined

According to requirejs/almond#62 (comment) , the anonymous declaration addressed by sheppard's PR is breaking things up...

I tried to revert the PR and the error isn't showing anymore.

How do you stand on this ?
Thanks.

@sheppard
Copy link
Contributor Author

Hi @Manumie,
As @jrburke noted in that discussion, Almond is meant to be used with modules that have already been through a build process that inserted module ids and dep arrays. This build process typically also puts everything into a single file. So, as you noted, it won't work with the source Leaflet file - this is by design.

I'm not familiar with Ember App Kit, but after a brief look it seems like you might be able to try one of the following:

  1. Figure out how to get Leaflet to be included as a module within Ember App Kit, so that it is built with the rest of the application into the final JavaScript file. From the looks of it you might need to add an ES6 module export, something like: export default L;. It looks like 3rd party module support is still being improved for EAK so it might be better to wait on this approach.
  2. Pre-build Leaflet using an AMD compiler like r.js:
node r.js -o name=leaflet out=leaflet-built.js optimize=none

If you run the above command in a folder with a copy of leaflet-src.js (renamed to leaflet.js), you should get an output file leaflet-built.js that is exactly the same, except that the anonymous declaration has been changed to a named one.

$ diff leaflet.js leaflet-built.js 
0a1
> 
18c19
<   define(L);
---
>   define('leaflet',[],L);

@Manumie
Copy link

Manumie commented Oct 16, 2013

Hi @sheppard

Thanks for your answer.

As you point out, the inclusion of 3rd party modules into Ember App Kit is a work in progress.

For now, even though one can manage 3rd party module packages with bower, one has to :

  1. build them locally.
    Usually, npm install inside the module directory does the job.
  2. include them manually in his app, within a special block that tells grunt (the builder) what assets to pack in the single file build.
<!-- build:js(.) assets/vendor.min.js -->
<script type="text/javascript" src="vendor/leaflet/dist/leaflet.min.js"></script>
<!-- endbuild -->

I'm sure the approach you recommend in your first point is the right one, but it's not ready yet.

I tried your second point, but I must be missing r.js or something.

For now, I edited vendor/leaflet/src/Leaflet.js and modified lines 13-15 :

    define('leaflet', [], function () { return L; });
//  define(L);

and then rebuilt leaflet with npm install

I understand this is awkward, but it's working.
I'll be looking at how things improve on module inclusion with Ember App.

Regards.

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.

3 participants