Hilary grunt by stuartf · Pull Request #2517 · oaeproject/3akai-ux

stuartf · 2012-12-12T15:05:23Z

production build in grunt

nicolaasmatthijs · 2012-12-12T17:45:19Z

The goal is for institutions to be able to add their own widgets next to oae-core, e.g. oae-gt. These widgets should be included in the production build as well.

nicolaasmatthijs · 2012-12-12T18:41:05Z

I just tried this, and during the build I got the following error:

/Users/nicolaas/Desktop/SakaiOAE/ui/newOAE/3akai-ux/target/optimized/ui/css/oae.main.css
Cannot inline css import, skipping: /shared/vendor/css/auth-buttons.css
/Users/nicolaas/Desktop/SakaiOAE/ui/newOAE/3akai-ux/target/optimized/ui/css/oae.main.css
Cannot inline css import, skipping: /shared/vendor/css/jquery.gritter.css
/Users/nicolaas/Desktop/SakaiOAE/ui/newOAE/3akai-ux/target/optimized/ui/css/oae.main.css
Cannot inline css import, skipping: /shared/vendor/css/jquery.autoSuggest.css

Also, when trying to run this in the browser, the UI is not loading at all. First of all, it can't find any of the above CSS files, as they should have been inlined.

It also can't find the oae.bootstrap.js file, which it tries to locate at /shared/oae/api/ab72e9d3.oae.bootstrap.cachcd382602.e.caced0d91c5.he.cache.js but actually just is in /shared/oae/api/ab72e9d3.oae.bootstrap.cache.js.

It looks like all of the individual api.js files are still there, which I think will cause too many requests for an initial pageload. In the old UI, all API files were merged into oae.dependencies. We should try to do something similar.

stuartf · 2012-12-12T21:03:30Z

all the individual api files are actually inlined into the [hash].oae.api.cache.js file, they remain in place so they can be referenced individually, but reading in oae.api won't result in additional calls. The old build left them in place as well. I'm looking into the filename mangling.

nicolaasmatthijs · 2012-12-12T23:04:40Z

I also just noticed that there is no node_modules folder in the optimized folder.

stuartf · 2012-12-13T18:52:03Z

I think this fixes all the listed issues, you'll have to use the version of grunt-ver in my master though until chrisdanford/grunt-ver#2 is resolved

nicolaasmatthijs · 2012-12-13T22:32:29Z

This should have been removed

nicolaasmatthijs · 2012-12-13T22:38:24Z

stuartf: can we point our package.json to your grunt-ver repo until the referenced issue is resolved?

stuartf · 2012-12-14T14:48:01Z

I've switched grunt-ver over to my repo in package.json and deleted the stray npmignore

bertpareyn · 2012-12-14T15:07:51Z

Can you take a look at the conflict?

… hilary-grunt Conflicts: nginx.conf ui/css/oae.main.css

nicolaasmatthijs · 2012-12-15T20:36:30Z

I fixed the merge conflict and pushed it to my hilary-grunt branch.

All of the files now seem properly hashed, but unfortunately this isn't working when tried in the browser. The first failure is in /shared/oae/api/oae.bootstrap.js, which still has ,require(["oae/api/oae.core.509094a8"] inside of it, causing a 404. It looks like most of the require statements have not been replaced by their hashed version.

Any chance you can have a look at this before Monday's demo?

mrvisser · 2013-02-04T20:40:15Z

This is a very clever solution to an otherwise hairy problem. I haven't looked too deep into the mechanics here yet, but am wondering if it is possible to take the cultureMap a step further and have it store the full path+hash, keyed by the locale, rather than just the hash. So the consuming code wouldn't always need to construct it with the hash?

The result would be that this consumer code would rather be:

var bundlesToLoad = ['/ui/bundles/default.properties', cultureMap.bundles[locale]];

And the hash = '.' + cultureMap.bundles[locale]; code wouldn't need to exist here or in l10n.

This only makes sense if the hash isn't needed for other purposes than just constructing the URL to the culture bundle.

I think the previous build system had a better approach for this problem. It would just hash the bundles folder and replace the bundles path inside of the codebase. As these would generate unique paths for the bundles, there was no need for something like the cultureMap. My preference would be to try to avoid the culturemap if possible.

How does the code get the hash value of the bundle directory?

It used to do an in-code string replacement of the unhashed path with the hashed path.

mrvisser · 2013-02-04T21:24:20Z

Sorry this hasn't had much review love over the past little while. Testing passes, just a few general comments I'd like addressed but otherwise looks good to merge. Good work @stuartf !

I imagine now that this tests out well, hashing would be next? Are there any known issues with the hashing procedure? Some initial testing shows that it doesn't work OOTB, but that isn't unexpected given we asked to have concat/minification working first.

stuartf · 2013-02-05T15:24:07Z

Pushed fixes for the review issues, I've left the dynamic hash resolution as is for now, because it might have to change when we fix the other hashing issues.

mrvisser · 2013-02-07T12:19:47Z

Doing a git grep "oae/", there are still define statements that use the full path and not the alias of the module. The ones that look to me like they may need to be changed are:

node_modules/oae-core/changepic/javascript/changepic.js:define(['jquery', 'underscore', 'oae/api/oae.core', 'jquery-plugins/imgareaselect/jquery.imgareaselect'], function($, _, oae) {
shared/oae/api/oae.api.i18n.js:define(['exports', 'jquery', 'oae/api/oae.api.config', 'oae/api/oae.api.util', 'oae.culture-map', 'jquery.properties-parser'], function(exports, $, configAPI, utilAPI, cultu
shared/oae/api/oae.api.l10n.js:define(['exports', 'underscore', 'oae/api/oae.api.config', 'oae.culture-map', 'globalize'], function(exports, _, configAPI, cultureMap) {
shared/oae/api/oae.api.util.js:define(['exports', 'jquery', 'underscore', 'oae/api/oae.api.i18n', 'jquery.validate', 'vendor/js/trimpath'], function(exports, $, _, i18nAPI) {
shared/oae/api/oae.api.widget.js:define(['exports', 'jquery', 'underscore', 'oae/api/oae.api.config', 'oae/api/oae.api.i18n', 'oae/api/oae.api.util', 'jquery.ba-bbq'], function(exports, $, _, configAPI, i

Should these change?

Conflicts: node_modules/oae-core/activity/js/activity.js

stuartf · 2013-02-08T21:31:33Z

Looking at the full paths vs aliases, if they're in oae.api they generally don't have to be changed, but I think they can be. The ones in changepic definitely need to be fixed, I'll get those fixed on Monday.

Conflicts: node_modules/oae-core/contentpreview/js/contentpreview.js

mrvisser · 2013-02-11T18:06:45Z

@stuartf I've submit a PR to your PR which fixes an issue with timeouts. Looks like the aliases for oae.api.util and oae.api.i18n were not the right way to go, but rather define oae.api directly as you suggested.

oae.api.i18n for imports

stuartf · 2013-02-11T19:14:45Z

@mrvisser I didn't take your patch unmodified, but the above should be equivalent.

mrvisser · 2013-02-11T21:49:26Z

@stuartf There were some more issues in the setup when merged with activities. The source of the problem was that there was a cross-dependency between infinitescroll and oae.core, since they require eachother. This was introduced by 4acd48b .

I've submit a PR to yours again with the changes.

Conflicts: shared/vendor/js/jquery-plugins/jquery.infinitescroll-sakai.js

stuartf · 2013-02-12T14:28:52Z

merged your commit

* Upgrading to require.js 2.1.4 * Fixing x-dependency issue in oae.api.util Conflicts: shared/oae/api/oae.bootstrap.js

nicolaasmatthijs · 2013-02-12T19:39:05Z

I just tried this locally, and it doesn't look like I can get this to work. I get the following error:

ReferenceError: exports is not defined     ...refix:"if (",suffix:") {",paramMin:1},"else":{delta:0,prefix:"} else {"},elseif:...            oae.api.js (line 1)
Error: Load timeout for modules: oae.api!_unnormalized2 http://requirejs.org/docs/errors.html#timeout      ...is.depMaps.push(a),r.completeLoad(u),c([u],i)}),n.load(e.name,c,i,o)})),r.enable...           require.js (line 1)

However, I can see the output, and it doesn't quite reflect the way in which the old production build used to work. I see the following HTTP requests when opening the page:

http://cambridge.oae.com/
http://cambridge.oae.com/ui/css/oae.main.css
http://cambridge.oae.com/shared/vendor/js/require.js
http://cambridge.oae.com/shared/oae/api/oae.bootstrap.js
http://cambridge.oae.com/shared/oae/api/oae.core.js
http://cambridge.oae.com/shared/oae/api/oae.api.js
http://cambridge.oae.com/shared/vendor/js/jquery.js
http://cambridge.oae.com/shared/vendor/js/underscore.js
http://cambridge.oae.com/shared/vendor/js/jquery-ui.custom.js
http://cambridge.oae.com/shared/vendor/js/l10n/globalize.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jqmodal.sakai-edited.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.ba-bbq.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.ba-hashchange.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.contentchange.sakai-edited.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.equal-height-columns.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.fileSize.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.form.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.gritter.sakai-edit.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.infinitescroll-sakai.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.properties-parser.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.serializeObject.js
http://cambridge.oae.com/shared/vendor/js/jquery-plugins/jquery.validate.js
http://cambridge.oae.com/shared/oae/api/oae.culture-map.js

We can see that there is only 1 CSS file (2 if there would be a page CSS file), which is the expected optimal behaviour. However, in the old production build, all of the JS libraries and OAE apis would be condensed into a single file (oae.dependencies), which significantly cuts down the number of requests and improves performance.

I'm fine with 2 separate files, one for oae.core and one for all 3rd party libraries, but I don't think we should have all of the files loaded individually. Is there anything we're currently doing with Require.JS that's preventing the old production build behaviour?

mrvisser · 2013-02-12T19:43:21Z

I discovered the "exports" issue while working on hashing locally, that was my bad as I cleaned things up for the PR.

I just finished up hashing locally, I'll look at doing this as well. Since hashing finished so quickly after the "define" functionality was fixed, I think I may just resubmit a PR with it all completed. Hopefully later today.

nicolaasmatthijs · 2013-02-12T19:44:19Z

Can you update the README to reflect these changes?

nicolaasmatthijs · 2013-02-12T19:59:11Z

Did a review as well.

mrvisser · 2013-02-13T11:36:19Z

I've addressed @nicolaasmatthijs 's comments in a fork in my hilary-grunt branch. I will close this and submit a new PR with Stuart's work here plus hashing and review fixes.

stuartf added 6 commits December 11, 2012 10:02

grunt build

259f729

remove unused tools and old build files

b347f29

css image embed

fbbf008

add git version file to built output

7c6e918

fix paths

92ae082

replace grunt-requirejs with grunt-contrib-requirejs

7f92728

stuartf mentioned this pull request Dec 12, 2012

Re-enable production build for new UI oaeproject/Hilary#285

Closed

nicolaasmatthijs reviewed Dec 12, 2012
View reviewed changes

nicolaasmatthijs closed this Dec 12, 2012

nicolaasmatthijs reopened this Dec 12, 2012

operate on all oae-* node modules

64fae99

fix hashing, exclusions, and css inlines, also render nginx.conf

37b0868

nicolaasmatthijs reviewed Dec 13, 2012
View reviewed changes

Comment thread .npmignore Outdated

nicolaasmatthijs Dec 13, 2012

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been removed

get patched grunt-ver remove npmignore

bf65a76

Nicolaas Matthijs added 2 commits December 15, 2012 17:54

Merge branch 'hilary-grunt' of git://github.com/stuartf/3akai-ux into…

0adf983

… hilary-grunt Conflicts: nginx.conf ui/css/oae.main.css

Fixed merge conflicts + new CSS file

95cab7b

stuartf added 2 commits December 18, 2012 14:08

move oae.core into requirejs paths, make build read paths from bootstrap

a56a9c4

replace paths in oae.bootstrap.js with hashed versions

afb3b93

fix typo in nginx warning message

dbd955d

mrvisser reviewed Feb 4, 2013
View reviewed changes

fix code review issues

4acd48b

stuartf added 2 commits February 7, 2013 12:34

Merge remote-tracking branch 'sakaiproject/Hilary' into hilary-grunt

099812a

Merge remote-tracking branch 'sakaiproject/Hilary' into hilary-grunt

019000a

Conflicts: node_modules/oae-core/activity/js/activity.js

stuartf added 2 commits February 11, 2013 09:55

Merge remote-tracking branch 'sakaiproject/Hilary' into hilary-grunt

1a62d09

Conflicts: node_modules/oae-core/contentpreview/js/contentpreview.js

Use aliased path for oae.core

fc5bba3

Use oae.api instead of individual oae.api.util and

6f5aa5b

oae.api.i18n for imports

infiniteScroll needs to require by path, not by module alias

1610996

Conflicts: shared/vendor/js/jquery-plugins/jquery.infinitescroll-sakai.js

Switch to using all aliases, rather than mix of alias and path

2b2e0a6

* Upgrading to require.js 2.1.4 * Fixing x-dependency issue in oae.api.util Conflicts: shared/oae/api/oae.bootstrap.js

nicolaasmatthijs reviewed Feb 12, 2013
View reviewed changes

Comment thread nginx.conf

nicolaasmatthijs Feb 12, 2013

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the README to reflect these changes?

mrvisser closed this Feb 13, 2013

Uh oh!

Conversation

stuartf commented Dec 12, 2012

Uh oh!

nicolaasmatthijs Dec 12, 2012

Choose a reason for hiding this comment

Uh oh!

nicolaasmatthijs commented Dec 12, 2012

Uh oh!

stuartf commented Dec 12, 2012

Uh oh!

nicolaasmatthijs commented Dec 12, 2012

Uh oh!

stuartf commented Dec 13, 2012

Uh oh!

nicolaasmatthijs Dec 13, 2012

Choose a reason for hiding this comment

Uh oh!

nicolaasmatthijs commented Dec 13, 2012

Uh oh!

stuartf commented Dec 14, 2012

Uh oh!

bertpareyn commented Dec 14, 2012

Uh oh!

nicolaasmatthijs commented Dec 15, 2012

Uh oh!

mrvisser Feb 4, 2013

Choose a reason for hiding this comment

Uh oh!

nicolaasmatthijs Feb 12, 2013

Choose a reason for hiding this comment

Uh oh!

mrvisser Feb 12, 2013

Choose a reason for hiding this comment

Uh oh!

nicolaasmatthijs Feb 12, 2013

Choose a reason for hiding this comment

Uh oh!

mrvisser commented Feb 4, 2013

Uh oh!

stuartf commented Feb 5, 2013

Uh oh!

mrvisser commented Feb 7, 2013

Uh oh!

stuartf commented Feb 8, 2013

Uh oh!

mrvisser commented Feb 11, 2013

Uh oh!

stuartf commented Feb 11, 2013

Uh oh!

mrvisser commented Feb 11, 2013

Uh oh!

stuartf commented Feb 12, 2013

Uh oh!

nicolaasmatthijs commented Feb 12, 2013

Uh oh!

mrvisser commented Feb 12, 2013

Uh oh!

nicolaasmatthijs Feb 12, 2013

Choose a reason for hiding this comment

Uh oh!

nicolaasmatthijs commented Feb 12, 2013

Uh oh!

mrvisser commented Feb 13, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants