Bugzilla – Full Text Bug Listing
| Summary: | Improve how the editor is using requirejs | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Felipe Heidrich <eclipse.felipe> | ||||||||
| Component: | Editor | Assignee: | Felipe Heidrich <eclipse.felipe> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | apeller, mihai.sucan, simon_kaegi | ||||||||
| Version: | 0.3 | ||||||||||
| Target Milestone: | 0.4 M2 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows 7 | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Felipe Heidrich
specifically, if arguments==3 then it is assumed that the first argument is an id, the second is dependencies, the third is the callback. I'm not sure if what you're using for your current 3rd argument is consistent with an id or not. I don't think Dojo would care if there are more than the specified 3 arguments, or if the id were null, so that's a possible workaround for you... but moving to requirejs sounds great, if you can do it. Almond seems to be what we need, instead of writing our own homegrown requirejs shim, in global.js. Can you guys import almond into the tree? Also, in Usage it is suggested that RequireJS is needed and that we should use an optimizer. Is that a requirement? At Mozilla we specifically do not minify/optimize the whole orion.js file - we want the source code to be readable. Optimization in AMD (e.g. requireJS) is optional. Straight deployment at development time was a requirement for the AMD design, but optimization for deployments are encouraged to reduce data transfer and minimize HTTP requests. James wrote up some suggestions here... http://requirejs.org/docs/whyamd.html#youcando It looks to me like you do need to use the optimizer (or something similar to ensure dependencies are visible) if you want to use almond. This is definitely the case if you want to use wrap=true to hide the implementation of define/require and only expose the Editor API as globals however we do not need to minify the resulting code. From what Mihai is looking for (e.g. no build required) we might still look at Almond but what's more important to me is that we are not creating yet another implementation of the AMD api unless we have no choice. If we really have to do a simple implementation then it really MUST match the AMD spec exactly but again this is my least favorite option. Oh, that usage section :) Sorry, I have no experience with Almond. Why wouldn't you want to use requirejs first, particularly for development, then maybe think about Almond for deployment to save some bytes? Created attachment 208342 [details]
example using global.js
Created attachment 208343 [details]
example using requirejs
Created attachment 208344 [details]
example using almond
I finally had time to try almond. This what I found: - does not define any names in the global namespace (differently than global.js). Which means the application code has to be written following AMD. (at this point, for us, I don't see why use almond over requirejs). When we added global.js the idea was to keep the old code (that rely on global names) running, or to allow application code that does not want to use AMD to use the textview/editor easily (only need is to include global.js). - requires all the scripts to be included in the html <script src="../../almond/almond.js"></script> <script src="../../orion/textview/eventTarget.js"></script> <script src="../../orion/textview/keyBinding.js"></script> <script src="../../orion/textview/textModel.js"></script> <script src="../../orion/textview/textView.js"></script> At this sense it is similar to global.js but different than requirejs. From what I read, users could use the optimizer to merge all the files (so that one one only script is included in the html). - does not support require.toUrl and other features as list in https://github.com/jrburke/almond in the Restrictions section. - to work we need to pass the id in all our define calls... Mihai, if we drop our global.js in favor of almond you will have to change your code. What do you think ? (In reply to comment #1) > I don't think Dojo would care if there are more than the specified 3 > arguments, or if the id were null, so that's a possible workaround for you... > but moving to requirejs sounds great, if you can do it. requirejs is not happy is id is null, undefined, or empty string. Anyhow, I read again http://requirejs.org/docs/api.html#modulename "These are normally generated by the optimization tool. You can explicitly name modules yourself, but it makes the modules less portable -- if you move the file to another directory you will need to change the name. It is normally best to avoid coding in a name for the module and just let the optimization tool burn in the module names. The optimization tool needs to add the names so that more than one module can be bundled in a file, to allow for faster loading in the browser." Not sure why is written that a named module is less portable, if I move a file to another directory I will have to fix all other files that depend on it (which is a lot more work and error prone that changing the name of the module itself). As a matter of fact, giving names to my modules should allow me to move the files around more easily (as I no longer use the path as the id). what am I missing ? (maybe in the during optimization phase) I am asking this because I see two possible fix for bug: 1. almond 2. passing id on all our define calls (removing the last argument which is a hack). I know that in Dojo, id is often used as the path. I don't know that there's any connection, though. Perhaps it's just an arbitrary string. I don't know that Dojo treats it that way. Would have to investigate. Why do you need an id at all? (In reply to comment #11) > I know that in Dojo, id is often used as the path. I don't know that there's > any connection, though. Perhaps it's just an arbitrary string. I don't know > that Dojo treats it that way. Would have to investigate. From what I understand: (1) if we specify an id for a module we need to use the same id in the dependencies list when including the module. (2) Unless we do something smart in the configuration object (like paths or packages) the id, if set, has to be the path. Otherwise the loader won't be able to find the script file. What I'm trying to get at is that we are already using paths in the dependencies list in the places the modules are used, thus using the path as id where the module is being defined should not make our modules less portable as suggested in: http://requirejs.org/docs/api.html#modulename By specifying ids in our modules definition I can: make our modules work with almond make our global.js follow the AMD standard (remove the last bogus parameter) I guess the question is, is there reason for not using ids when defining modules ? (In reply to comment #9) > Mihai, if we drop our global.js in favor of almond you will have to change your > code. What do you think ? Please see: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/source-editor-orion.jsm#190 That's the latest integration code working with Orion from the 9th of December 2011, commit d8a6dc01d9c561d6eb99f03b64c8c78ce785c59d. To see what is included in the big orion.js, see: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/orion/Makefile.dryice.js I do require() calls that assume the modules are already loaded, no async waiting for the deps. That's easier to maintain for me. There's not much point in having the require() async call when we know that orion.js holds *all* of the modules already. If almond allows me to do this as easily as with the global.js shim - that's exactly what I need. Will you include almond.js in the codebase? Or will I have to take that code separately? To us there's not much relevance to how define() calls happen throughout the Orion code, as long as the ease of use mentioned above stays valid. Thank you! (In reply to comment #12) > > I guess the question is, is there reason for not using ids when defining > modules ? Felipe please go ahead and use an id for now as using no id is guidance not mandatory and in this case not a huge deal. Frankly in most cases it will not be a big deal. The reason folk prefer not using ids is it makes it much easier for people to take a code base and reuse it in their own hierarchy. To do this effectively though requires one to use relative paths in dependencies and to be honest I'm not convinced this is a good use of our time at this point. http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=91ec711927c5b3a35848c859fda5fcc0252d50ec Adam, this fixes the problem you had. (In reply to comment #13) > If almond allows me to do this as easily as with the global.js shim - that's > exactly what I need. I tested: let TextModel = window.require("orion/textview/textModel").TextModel; with almond and it worked fine. I think you can move from global.js to almond just by changing ORION_EDITOR + "/orion/textview/global.js", to ORION_EDITOR + "/almond/almond.js", (with almond all scripts are loaded up front, I don't think there is any async loading happening). > Will you include almond.js in the codebase? Or will I have to take that code > separately? Simon ? We already include requirejs, so I don't think it would be a problem to include almond... Mihai, is it problematic for you to download amond.js from github ? I'm keeping this problem open till we decide to keep or remove global.js (In reply to comment #16) > (In reply to comment #13) > > If almond allows me to do this as easily as with the global.js shim - that's > > exactly what I need. > > I tested: > let TextModel = window.require("orion/textview/textModel").TextModel; > with almond and it worked fine. > > I think you can move from global.js to almond just by changing > ORION_EDITOR + "/orion/textview/global.js", > to > ORION_EDITOR + "/almond/almond.js", > > (with almond all scripts are loaded up front, I don't think there is any async > loading happening). Yep, pretty much as I expected. Thank you for testing! > > Will you include almond.js in the codebase? Or will I have to take that code > > separately? > > Simon ? > We already include requirejs, so I don't think it would be a problem to include > almond... > > Mihai, is it problematic for you to download amond.js from github ? Technically, I can do that, but I will have to ask my manager. I need to also check for license concerns. > I'm keeping this problem open till we decide to keep or remove global.js If we are to move to almond/requirejs I suggest that global.js to be removed. Is it possible to keep it (if it's still usable) until we make the switch to almond? Thank you! (In reply to comment #17) > Will you include almond.js in the codebase? Talked to Simon, we are not going to include almond because we do not use it in Orion. > Technically, I can do that, but I will have to ask my manager. I need to also > check for license concerns. Let me know if that is an option. > If we are to move to almond/requirejs I suggest that global.js to be removed. Agreed. > Is it possible to keep it (if it's still usable) until we make the switch to > almond? Of course, let me know when you are ready. (In reply to comment #18) > (In reply to comment #17) > > Will you include almond.js in the codebase? > > Talked to Simon, we are not going to include almond because we do not use it in > Orion. That makes sense. > > Technically, I can do that, but I will have to ask my manager. I need to also > > check for license concerns. > > Let me know if that is an option. > > > Is it possible to keep it (if it's still usable) until we make the switch to > > almond? > > Of course, let me know when you are ready. I asked people around and I learned we do have a minimal requirejs shim in our Firefox codebase. I'll look into using that and updating that if needed, to work with Orion. As I understand, it's good to go as it is. I would say you can remove global.js now. I will not get back to update Orion from upstream too soon (it will take at least one more month). I am "booked" for some Web Console work. Thank you! We are actually using global.js in three places in our code base: bundles/org.eclipse.orion.client.core/web/plugins/webEditingPlugin.html bundles/org.eclipse.orion.client.editor/web/examples/editor/embeddededitor.html bundles/org.eclipse.orion.client.editor/web/examples/editor/minimaleditor.html it is trivial to update the two examples to use requirejs, as for the webEditingPlugin I'm not sure what to do. Simon ? are plugins suppose to use requirejs now ? (In reply to comment #20) > it is trivial to update the two examples to use requirejs, as for the > webEditingPlugin I'm not sure what to do. > > Simon ? are plugins suppose to use requirejs now ? See Bug 362505 and Bug 362808 It would be good to have our plugins built to reduce load time, so... yes. One issue is plugin.js as it is currently not an amd component but for now we can use its global name. (In reply to comment #22) > It would be good to have our plugins built to reduce load time, so... yes. One > issue is plugin.js as it is currently not an amd component but for now we can > use its global name. I saw that, is there a bug for this already ? I'm closing this bug fixed as the code is in. There is still the problem with webEditingPlugin (and using requirejs for plugins) that should be tracked in a different problem report (I'll add a note to the bug to make sure global.js gets deleted at the end of the process). |