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.

Sign up for GitHub

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

Jump to bottom

Update AMD loaders #3107

Merged
merged 4 commits into from
Oct 23, 2015
Merged

Update AMD loaders #3107

merged 4 commits into from
Oct 23, 2015

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Oct 18, 2015

Update requirejs from 2.1.9 to 2.1.20.
Update almond from 0.2.6 to 0.3.1.

Someone related to #3106, but this can go into master at any time.

@mramato
Copy link
Contributor Author

mramato commented Oct 20, 2015

Don't merge this yet, there may be an issue with minified Workers that I'm looking into.

@mramato
Copy link
Contributor Author

mramato commented Oct 20, 2015

It looks like the upgrade causes all release tests that use Specs/TestWorkers to fail. @shunter were we abusing the loader at all in any way that would cause this to fail? After doing a build the path ends up being something like Specs./Specs/TestWorkers/returnParameters.js instead of ./TestWorks/returnParameters or Specs/TestWorkers/returnParameters (I'm not sure which one is correct(.

@mramato
Update AMD loaders
77d22e0 
Update `requirejs` from 2.1.9 to 2.1.20.
Update `almond` from 0.2.6 to 0.3.1.
@mramato
Copy link
Contributor Author

mramato commented Oct 22, 2015

@shunter I noticed cesiumWorkerBootstrapper has an entire copy of requirejs in it (which I hadn't updated but assume we should). There's definitely an issue between either that, or something we are doing in TaskProcessor that are causing a few Spec test failures. Cesium itself seems to work fine, but some worker-related tests are failing. Any ideas?

@shunter
Copy link
Contributor

shunter commented Oct 23, 2015

I tried this branch and everything seems to be working. TaskProcessor specs are passing for me in normal mode, combined mode, and minified mode.

Aside from that, yes, you should update the copy of require.js in the bootstrapper.

@shunter
Copy link
Contributor

shunter commented Oct 23, 2015

Nevermind, I just noticed that your force push made my git update fail. I'll try again.

@mramato
Copy link
Contributor Author

mramato commented Oct 23, 2015

Nevermind, I just noticed that your force push made my git update fail. I'll try again.

Sorry about that, I've been a little force push happy lately to clean up my own commit histories.

@shunter
Fix TestWorkers in TaskProcessorSpec.
0333e2f 
Recent changes (well, over a year ago) to RequireJS broke the way we were locating the Source directory in both normal and combined test modes. Instead, we now configure RequireJS with an additional path "Source" which always points to the Source directory, regardless of mode.

This path should not be used from other specs, because it will bypass the stubs in the combined test mode. It is specifically intended for the TaskProcessor specs, because the TestWorkers need to always load using the individual modules in Source.
@shunter
Update RequireJS to 2.1.20
eb968a1
@shunter
Copy link
Contributor

shunter commented Oct 23, 2015

I fixed the problems with workers in the tests, and updated the version of RequireJS in the bootstrapper. Merging.

shunter added a commit that referenced this pull request Oct 23, 2015
@shunter
Merge pull request #3107 from AnalyticalGraphicsInc/update-requirejs
f5ed115 
Update AMD loaders
@shunter shunter merged commit f5ed115 into master Oct 23, 2015
@shunter shunter deleted the update-requirejs branch October 23, 2015 22:54
@mramato
Copy link
Contributor Author

mramato commented Oct 23, 2015

Awesome, thanks @shunter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mramato @shunter