Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#15444 closed defect (fixed)

[patch] [cla] Consider supporting module.config(), modifying dojo/has

Reported by: James Burke Owned by: Rawld Gill
Priority: low Milestone: 1.8
Component: Core Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description

requirejs 2.0 supports module.config() now, based on the discussion and suggestion from Rawld on the amd-implement list. I need to work up a real page on the amdjs wiki since we all seemed to be in agreement.

In the meantime though, it would be great if Dojo could support module.config().

At a minimum though, it would be nice if dojo/has checked for it, since the way it grabs config now makes it difficult for the requirejs optimizer to do fully optimized builds for dojo. I believe the attached patch is all that would be needed for this first step and to allow better optimized build with requirejs.

I am willing to do testing via requirejs if this lands.

If there are other modules that use config in a similar way, it would be good to have them match this pattern. So far I'm only aware of dojo/has though.

Attachments (2)

has.patch (897 bytes) - added by James Burke 5 years ago.
possible patch file for dojo/has
15444.patch (522 bytes) - added by James Burke 5 years ago.
better has.js patch for seeding cache from config, supersedes older has.patch

Download all attachments as: .zip

Change History (12)

Changed 5 years ago by James Burke

Attachment: has.patch added

possible patch file for dojo/has

comment:1 Changed 5 years ago by bill

Milestone: tbd1.8
Owner: set to Rawld Gill
Status: newassigned
Summary: Consider supporting module.config(), modifying dojo/has[patch] [cla] Consider supporting module.config(), modifying dojo/has

comment:2 Changed 5 years ago by Colin Snover

Milestone: 1.82.0

1.8 is frozen. Move all enhancements to next release. If you need an exemption from the freeze for this ticket, contact me immediately.

comment:3 Changed 5 years ago by James Burke

I apologize for not following up on this sooner. Is it too late for 1.8 train for the attached patch? I believe it is low impact. I just got pinged today again about not being able to do requirejs builds with dojo code, and traced it to the inability for the requirejs build to pass config to the dojo/has plugin.

I completely understand if it is too late though. I should have followed up sooner. If there is a possibility of allowing it, I can do the commit, run some dojo tests to make sure it did not break anything.

comment:4 Changed 5 years ago by James Burke

I put the wrong attachment to this ticket, and doing some tests with the 1.8b1, looks like there might be some other issues too. I'm going to sort them out, then post back to this ticket with the findings, as the fix may be more involved than before.

So, hold off on looking at the current patch in this ticket.

comment:5 Changed 5 years ago by Rawld Gill

Priority: undecidedlow

Changed 5 years ago by James Burke

Attachment: 15444.patch added

better has.js patch for seeding cache from config, supersedes older has.patch

comment:6 Changed 5 years ago by James Burke

Just uploaded a patch that seeds has.js with the config cache for a build. Discard the older has.patch.

Even with this change though, it is difficult to work out what set of has tests to preseed to allow the build to include all the browser modules. Still looking into that.

comment:7 Changed 5 years ago by James Burke

Maybe part of the issue: I try passing 'host-browser' as true to the config to get it to maybe include the other browser modules when doing build tracing, but then has.js runs some other very specific browser tests when that test is true, which then breaks. If I do a conditional around those tests, it still does not seem to be enough to end up with say, all the base modules for the browser in the build (still see dynamic loads after the build).

comment:8 Changed 5 years ago by Rawld Gill

Resolution: fixed
Status: assignedclosed

In [29186]:

fixes amd config / allow foreign loaders to init has cache; thanks james burke; fixes #15444; refs #15616; !strict

comment:9 in reply to:  7 Changed 5 years ago by Rawld Gill

Replying to jburke:

Maybe part of the issue: I try passing 'host-browser' as true to the config to get it to maybe include the other browser modules when doing build tracing, but then has.js runs some other very specific browser tests when that test is true, which then breaks. If I do a conditional around those tests, it still does not seem to be enough to end up with say, all the base modules for the browser in the build (still see dynamic loads after the build).

It may help to check out http://trac.dojotoolkit.org/browser/dojo/util/trunk/build/buildControlDefault.js#L42 which is the default set of static has test values we are using in the builder.

If you still hit a block, let me know what modules are dynamically loading...maybe I'll be able to give some better hints.

comment:10 Changed 5 years ago by Rawld Gill

Milestone: 2.01.8
Type: enhancementdefect
Note: See TracTickets for help on using tickets.