Opened 8 years ago

Closed 8 years ago

#14831 closed defect (fixed)

dojo-xhr-factory should not rely on dojo-sync-loader

Reported by: Colin Snover Owned by: Rawld Gill
Priority: blocker Milestone: 1.7.3
Component: Loader Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by Colin Snover)

This reliance causes an inadvertent regression when using 1.7.2 with the default dojo boilerplate build profile.

Steps to reproduce:

  1. git clone --recursive https://csnover@github.com/csnover/dojo-boilerplate.git
  2. git checkout 6e48a3af1c
  3. ./build.sh
  4. visit dist/index.html

Expected: Loads successfully
Actual: “a._xhrObj is undefined”

Worked with 1.7.1. Workaround is to ensure dojo-xhr-factory is false when dojo-sync-loader is false.

Change History (11)

comment:1 Changed 8 years ago by Colin Snover

Description: modified (diff)

comment:2 Changed 8 years ago by Colin Snover

Description: modified (diff)

comment:3 Changed 8 years ago by Rawld Gill

Resolution: worksforme
Status: newclosed

Add

    'dojo-xhr-factory':0

to the staticHasFeatures set in app.profile.js and this problem goes away. The default build switches build for the sync loader in 1.x line, and this profile is building for AMD.

I think we need to add some build profiles for AMD builds, but that's a different issue.

That it worked in 1.7.1 was chance.

comment:4 Changed 8 years ago by Colin Snover

Since this isn’t the first time I’ve run into a has.js flag dependency like this, I’m wondering if there should be a way to mark staticHasFlags as dependent on other flags or something in order to prevent these kinds of issues, though I’m not sure it would ultimately be worth the effort, and it would of course introduce the annoying possibility of circular references. Thoughts? I could open another ticket for that if it even seems like a good idea to consider.

comment:5 Changed 8 years ago by ben hockey

Resolution: worksforme
Status: closedreopened

i'm having this exact same problem. i don't think that it should be considered "chance" that removing the sync loader happened to work in 1.7.1 but does not work now - i think it's a regression. a simple defensive change to dojo/_base/xhr would help here

if(has("dojo-xhr-factory")  && require.getXhr){
        dojo._xhrObj = require.getXhr;
} else ...

comment:6 Changed 8 years ago by ben hockey

fyi - 'dojo-v1x-i18n-Api' and require.isXdUrl used by dojo/i18n have a similar interdependency with 'dojo-sync-loader'. are there any other flags like this? we should at least identify and document them somehow.

comment:7 Changed 8 years ago by ben hockey

Priority: undecidedblocker

i added

'dojo-v1x-i18n-Api':0

to my profile as an attempt to work around require.isXdUrl not being available after the sync loader is removed but my build output depends on the v1x i18n api due to the call to _preloadLocalizations in the preload call added by util/build/transforms/writeAmd. is there any workaround for this that allows me to remove the sync loader?

moving priority to blocker since we should fix this and backport to 1.7 branch since it's broken there

EDIT: corrected reference to util/buildscripts/jslib/i18nUtil as util/build/transforms/writeAmd

Last edited 8 years ago by ben hockey (previous) (diff)

comment:8 Changed 8 years ago by ben hockey

Milestone: tbd1.7.3

comment:9 Changed 8 years ago by Rawld Gill

Status: reopenedassigned

comment:10 Changed 8 years ago by Rawld Gill

Some kinda interesting tradeoffs are seen in this ticket.

@neonstalwart:my instinct is that the suggested change at comment5 adds cruft which is counter why we're using the has feature test here, and, therefore, the flag should just be set correctly in the build profile. Also, there is no guarantee as to the semantics of require.getXhr iff dojo-xhr-factory is not true. However, properly setting dojo-xhr-factory to false---which is what would be done when creating a really tight build--will trim the cruft. Therefore, I'm committing this suggestion in the interest of doing the least surprising thing.

@csnover: I certainly agree that the sync loader shouldn't be required just because a call to _proloadLocalizations exists. I happened to have fixed that at [28365], which will be backported. I'll track this issue at #14092.

comment:11 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: assignedclosed

In [28398]:

improved sniff for dojo xhr factory; fixes #14831; refs #14092; !strict

Note: See TracTickets for help on using tickets.