Move AMD registration to end of file.#584
Conversation
This better represents when jQuery is completely defined and is compatible with more AMD loaders.
|
Thanks for this! I don't think the unit test needs to be removed though. It can stay in core.js. All jQuery modules in the test suite are executed before any tests are run so the define will still work. As for testing, perhaps we should include require and curl in the test suite to have tests to show we are satisfying the loaders with our amd registration. |
|
@timmywil the problem for me is that outro.js does not show up in test/index.html (rightly so, it is a partial JS file) so I could not see how the test could stay in the core.js test file. I'm open to suggestions if you do want test coverage in the jquery tree though. |
|
Brain fart Yes of course outro isn't loaded. Perhaps we should have a global.js or something for including the exposure in the test suite. |
|
Can I jump in here for a minute? I've actually needed the test suite to load all of jQuery as one file for a different ticket that I'm working on. Rather than introducing another file that would need to be loaded everywhere in the test suite, I went ahead and made a file in test/data/ called concatenated.php which provided the concatenated src, intro.js and outro.js and all, in the same way as the Makefile does. The downside of this is that the tests would need to be run from outside of the file:// protocol context. The test suite already has an isLocal variable to wrap tests that need php. I was wondering if this would make sense for testing this as well. Having a way to load a potential dist version of jQuery from the test suite sounds useful. 6000 tests so far haven't needed it, but maybe it's time? |
|
I think it's still best to keep it loading as is. It can be very helpful to only load certain modules or rearrange them in certain cases without the need to edit their arrangement in yet another file. Separating the exposure of jQuery to exports and/or the window from the outro makes sense. |
|
After talking with @timmywil, he suggested creating a a new file to hold this stuff instead of having it in outro.js so it can be tested. Closing this pull request, will open a new one with that other approach. |
|
closed |
This is in reference to ticket 10687:
This better represents when jQuery is completely defined and is compatible with more AMD loaders.
It meant the unit test had to be removed since the AMD registration moved to outro.js, and as far as I know there are no tests just for outro. However I have tested this file and it does work in requirejs. @unscriptable should probably also give it a whirl in curl to confirm the fix.
If it makes sense, I can do a test that can be placed in the test/ directory and load the dist/jquery.js file for the test, but I am not sure if there is precedent for that, or if it is good enough that some AMD folks have confirmed the fix.