Opened 8 years ago

Closed 8 years ago

#12329 closed defect (fixed)

[calc] project uses deprecated define() syntax?

Reported by: dante Owned by: dante
Priority: high Milestone: 1.6
Component: General Version: 1.6.0rc1
Keywords: Cc: ben hockey, Rawld Gill, Kris Zyp, Douglas Hays
Blocked By: Blocking:

Description

Just running the rc for 1.6.0 and the post-build files that fail to convert from AMD format are present.

Discovered the cause of #12301 was the syntax used in dojox/calc for AMD loader:

define(["dojo", "dijit", etc], function(dojo){ 

Which the regexp in the build and doc parser don't unwrap properly.

The full list is as follows:

dante@bender:/srv/www/vhosts/download.dojotoolkit.org/release-1.6.0rc1/dojo-release-1.6.0rc1> grep "define(" * -R
dijit/lib/main.js:define(["dojo"], function(dojo) {
dojo/lib/backCompat.js:define(["require", "dojo/_base/_loader/bootstrap"], function(require, dojo){
dojo/lib/kernel.js:define(["dojo/_base/_loader/hostenv_browser"], function(dojo_){
dojo/lib/main-browser.js:define("dojo", [
dojo/lib/plugins/i18n.js:define(["dojo"], function(dojo) {
dojo/lib/plugins/text.js:define(["dojo", "dojo/cache"], function(dojo){
dojo/tests/_base/_loader/a.js:define({
dojo/tests/_base/_loader/modules/anon.js:define(["../a", "./wrapped"], function (a, wrapped) {
dojo/tests/_base/_loader/modules/data.js:define({
dojo/tests/_base/_loader/modules/factoryArity.js:define(function() { return {i : 5} ; });
dojo/tests/_base/_loader/modules/full.js:define("dojo/tests/_base/_loader/modules/full", ["./anon", "../a", "./wrapped", "require"], function (anon, a, wrapped, require) {
dojo/tests/_base/_loader/modules/wrapped.js:define(function (require, exports, module) {
dojo/tests/_base/_loader/modules.js:define("dojo/tests/_base/_loader/modules", ["require", "dojo/_base/connect", "./modules/anon","./modules/wrapped","dojo/tests/_base/_loader/modules/full","./modules/data",".modules/factoryArity"], function(require, connect, anon, wrapped, factoryArity){
dojo/tests/_base/AMD-build-transform/t3.js:define(["your/module", "his/module"], function(yours, his) {
dojo/tests/amd/backCompat.js:define(
dojo/tests/behavior.html:			define("dojo/tests/bahavior/script", ["dojo", "dojo/behavior"], function(dojo) {
dojo/tests/currency.js:  define(["dojo", "dojo/currency", "plugin/i18n"], function(dojo){
dojo/tests/currency.js:            define(deps, function(dojo){
dojo/tests/date/locale.js:            define(deps, function(){
dojo/tests/i18n.js:  define(["dojo", "plugin/i18n"], function(dojo) {
dojo/tests/i18n.js:          define([dojo.getL10nName("dojo/tests", "salutations", locale)], function(bundle){
dojo/tests/number.js:            define(deps, function(){
dojo/tests/parser.html:		define("dojo/tests/parser/script", ["dojo", "dojo/parser", "doh/runner"], function(dojo) {
dojox/calc/_Executor.js:define(["dojo", "dijit/_Templated", "dojox/math/_base"], function(dojo) {
dojox/calc/FuncGen.js:define(["dojo", "dijit/_Templated", "dojox/math/_base", "dijit/dijit", "dijit/form/ComboBox", "dijit/form/SimpleTextarea", "dijit/form/Button", "dojo/data/ItemFileWriteStore"], function(dojo) {
dojox/calc/Grapher.js:define(["dojo", "dijit/_Templated", "dojox/math/_base", "dijit/dijit", "dijit/form/DropDownButton", "dijit/TooltipDialog", "dijit/form/TextBox", "dijit/form/Button", "dijit/form/ComboBox", "dijit/form/Select", "dijit/form/CheckBox", "dijit/ColorPalette", "dojox/charting/Chart2D", "dojox/charting/themes/Tufte", "dojo/colors"], function(dojo) {
dojox/calc/GraphPro.js:define(["dojo", "dojox/calc/Standard", "dijit/dijit", "dijit/form/ComboBox", "dijit/form/Select", "dijit/form/CheckBox", "dijit/ColorPalette", "dojox/charting/Chart2D", "dojox/calc/Grapher", "dojox/layout/FloatingPane", "dojox/charting/themes/Tufte", "dojo/colors"], function(dojo) {
dojox/calc/Standard.js:define(["dojo", "dijit/_Templated", "dojox/math/_base", "dijit/dijit", "dijit/Menu", "dijit/form/DropDownButton", "dijit/TooltipDialog", "dijit/form/TextBox", "dijit/form/Button", "dojox/calc/_Executor"], function(dojo) {
dojox/calc/toFrac.js:define(["dojo"], function(dojo) {
util/doh/runner.html:						define("doh", [], doh);

The dojox.calc items are fixed by my patch for #12301 which I'll probably just commit. The rest are in lib/ or tests/ folders.

Should they be unwrapping properly? I noticed dijit/lib/main.js initially was failing my doc-parsing unwrap because it uses an "AMD ID format" that doesn't match the stuff in buildutil or docscripts

Change History (10)

comment:1 Changed 8 years ago by bill

Cc: ben hockey Rawld Gill added
Keywords: neonstalwart rawld removed

Good catch. Note that technically that's not a deprecated syntax, it's the syntax we will use in the future, but it doesn't work with the 1.6 build tool.

comment:2 Changed 8 years ago by bill

PS: the test files shouldn't be using define() at all, since they aren't defining anything. The correct thing to use is require(), but that's not supported in 1.6.

comment:3 Changed 8 years ago by ben hockey

Cc: Kris Zyp added
dijit/lib/main.js:define(["dojo"], function(dojo) {
dojo/lib/backCompat.js:define(["require", "dojo/_base/_loader/bootstrap"], function(require, dojo){
dojo/lib/kernel.js:define(["dojo/_base/_loader/hostenv_browser"], function(dojo_){
dojo/lib/main-browser.js:define("dojo", [
dojo/lib/plugins/i18n.js:define(["dojo"], function(dojo) {
dojo/lib/plugins/text.js:define(["dojo", "dojo/cache"], function(dojo){

as far as the build is concerned, all of these files above shouldn't matter if they aren't transformed because they shouldn't be included in any project that uses dojo as the loader and so they won't be broken. they are specifically for use with an AMD loader only. are you saying that they're breaking something or are you just pointing out that they didn't convert?

dojo/tests/_base/_loader/a.js:define({
dojo/tests/_base/_loader/modules/anon.js:define(["../a", "./wrapped"], function (a, wrapped) {
dojo/tests/_base/_loader/modules/data.js:define({
dojo/tests/_base/_loader/modules/factoryArity.js:define(function() { return {i : 5} ; });
dojo/tests/_base/_loader/modules/full.js:define("dojo/tests/_base/_loader/modules/full", ["./anon", "../a", "./wrapped", "require"], function (anon, a, wrapped, require) {
dojo/tests/_base/_loader/modules/wrapped.js:define(function (require, exports, module) {
dojo/tests/_base/_loader/modules.js:define("dojo/tests/_base/_loader/modules", ["require", "dojo/_base/connect", "./modules/anon","./modules/wrapped","dojo/tests/_base/_loader/modules/full","./modules/data",".modules/factoryArity"], function(require, connect, anon, wrapped, factoryArity){
dojo/tests/_base/AMD-build-transform/t3.js:define(["your/module", "his/module"], function(yours, his) {
dojo/tests/amd/backCompat.js:define(

rawld may have more to say about the files in the block above. i think they are specifically for testing AMD. in fact, some of them are tests that are meant to fail and it is right that they didn't get transformed.

dojo/tests/behavior.html:			define("dojo/tests/bahavior/script", ["dojo", "dojo/behavior"], function(dojo) {
dojo/tests/currency.js:  define(["dojo", "dojo/currency", "plugin/i18n"], function(dojo){
dojo/tests/currency.js:            define(deps, function(dojo){
dojo/tests/date/locale.js:            define(deps, function(){
dojo/tests/i18n.js:  define(["dojo", "plugin/i18n"], function(dojo) {
dojo/tests/i18n.js:          define([dojo.getL10nName("dojo/tests", "salutations", locale)], function(bundle){
dojo/tests/number.js:            define(deps, function(){
dojo/tests/parser.html:		define("dojo/tests/parser/script", ["dojo", "dojo/parser", "doh/runner"], function(dojo) {

the block above i'm uncertain about - maybe rawld or kris knows more...

dojox/calc/_Executor.js:define(["dojo", "dijit/_Templated", "dojox/math/_base"], function(dojo) {
dojox/calc/FuncGen.js:define(["dojo", "dijit/_Templated", "dojox/math/_base", "dijit/dijit", "dijit/form/ComboBox", "dijit/form/SimpleTextarea", "dijit/form/Button", "dojo/data/ItemFileWriteStore"], function(dojo) {
dojox/calc/Grapher.js:define(["dojo", "dijit/_Templated", "dojox/math/_base", "dijit/dijit", "dijit/form/DropDownButton", "dijit/TooltipDialog", "dijit/form/TextBox", "dijit/form/Button", "dijit/form/ComboBox", "dijit/form/Select", "dijit/form/CheckBox", "dijit/ColorPalette", "dojox/charting/Chart2D", "dojox/charting/themes/Tufte", "dojo/colors"], function(dojo) {
dojox/calc/GraphPro.js:define(["dojo", "dojox/calc/Standard", "dijit/dijit", "dijit/form/ComboBox", "dijit/form/Select", "dijit/form/CheckBox", "dijit/ColorPalette", "dojox/charting/Chart2D", "dojox/calc/Grapher", "dojox/layout/FloatingPane", "dojox/charting/themes/Tufte", "dojo/colors"], function(dojo) {
dojox/calc/Standard.js:define(["dojo", "dijit/_Templated", "dojox/math/_base", "dijit/dijit", "dijit/Menu", "dijit/form/DropDownButton", "dijit/TooltipDialog", "dijit/form/TextBox", "dijit/form/Button", "dojox/calc/_Executor"], function(dojo) {
dojox/calc/toFrac.js:define(["dojo"], function(dojo) {

it looks like dojox/calc is certainly broken - if there are no objections, i'll fix it.

util/doh/runner.html:						define("doh", [], doh);

this is how we get doh to work with AMD.

in summary, the only things that might need addressing are the dojo tests that i've said kris or rawld may be able to comment on and dojox/calc. i'll update dojox/calc and see what kris or rawld have to say about the dojo tests that don't transform.

comment:4 Changed 8 years ago by Adam Peller

Cc: Douglas Hays added

comment:5 Changed 8 years ago by dante

@kris - dojox.calc is what started this. I have a patch to fix it on #12301, just wanted to make sure I was being sane. Happy to commit it. It is definitely broken in rc1.

The only reason I question the dojo/lib dijit/lib is because the doc parser is attempting to read them, and is unable to unwrap them. It isn't dying, just failing to grok any information for dojo.lib ... I suspect there will be a API entry for an empty object called dojo.lib (and dijit.lib) when we run the 1.6 API doc export.

It is not breaking anything like the build or doc export. Just providing false positives on objects in our namespace.

comment:6 in reply to:  5 Changed 8 years ago by ben hockey

@kris - dojox.calc is what started this. I have a patch to fix it on #12301, just wanted to make sure I was being sane. Happy to commit it. It is definitely broken in rc1.

maybe you meant me rather than kris... the dojox-calc patch looks fine. an alternative would have been to add a line comment on the first line like:

// AMD-ID "dojox/calc/toFrac"
define([...]

using anonymous modules is more portable but at this point i don't think it really matters too much either way.

The only reason I question the dojo/lib dijit/lib is because the doc parser is attempting to read them, and is unable to unwrap them. It isn't dying, just failing to grok any information for dojo.lib ... I suspect there will be a API entry for an empty object called dojo.lib (and dijit.lib) when we run the 1.6 API doc export.

It is not breaking anything like the build or doc export. Just providing false positives on objects in our namespace.

i'm not sure how we can avoid this. i assume that even if we wrote those so that they transformed, we'd still get those false positives in the namespace simply because the files exist? i don't really know how the doctool works so not sure how we can trick it into ignoring these. any suggestions?

comment:7 Changed 8 years ago by dante

if we wrote them in a way they got transformed we could put actual information in those objects.

/*=====
dojo.lib = {
    // summary: The Dojo AMD package thinger. Not for real use outside of packagers. Or whatever the real reason is, I'm a bit fuzzy myself.
};
=====*/

At least then it wouldn't be mystic.

I notice the dijit/lib.js doesn't use 'AMD-ID = "something"' it says

AMD module = "dijit"

or some such. Though I did try unwrapping from AMD-ID's, seems my docparser unwrapper is broken in that regard. Will double check.

comment:8 Changed 8 years ago by Rawld Gill

re AMD pragmas:

// AMD-ID "foo/bar" is a pragma to help the transform with anonymous AMD modules. Although requested, it's never actually used in dojo or dijit (other than a couple of tests to show that it works).

// AMD module id = foo/bar is simple commentary and serves no other purpose.

re dojo/lib, dijit/lib:

These are the beginnings of normal, AMD-compliant packages for dojo and dijit. As Kris said, they should never be transformed back to dojo.require/provide. They do not introduce any new features; they are only shims to make existing features work when dojo/dijit is loaded by an AMD loader. As such, I'd recommend not putting anything in the API docs about them.

Re use of define in the tests: These are artifacts of moving dojo-sie into dojo last fall when the API was changing coupled with the way the AMD simulation works with the 1.6 sync loader. Simulated define in the 1.6 sync loader unconditionally and immediately runs the factory. This has the net effect of working very much like AMD require. Bill made me aware of this several weeks ago, but since (1) everything seemed to be working, (2) we were in beta, and (3) the tests need some significant work to fix correctly for use with AMD, I elected to wait until after 1.6 to address.

comment:9 Changed 8 years ago by dante

@rcgill - the problem is there is no way to tell the parser to ignore a clearly "public" object stubbed on a namespace. It is going to show up regardless, so I was wondering if a quick note in the form of api docs would prevent confusion.

comment:10 Changed 8 years ago by dante

Resolution: fixed
Status: newclosed

(In [23899]) fixes #12329 - makes dojox.calc amd unwrappable

Note: See TracTickets for help on using tickets.