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: jburke Owned by: rcgill
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 jburke 5 years ago.
possible patch file for dojo/has
15444.patch (522 bytes) - added by jburke 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 jburke

possible patch file for dojo/has

comment:1 Changed 5 years ago by bill

  • Milestone changed from tbd to 1.8
  • Owner set to rcgill
  • Status changed from new to assigned
  • Summary changed from Consider supporting module.config(), modifying dojo/has to [patch] [cla] Consider supporting module.config(), modifying dojo/has

comment:2 Changed 5 years ago by csnover

  • Milestone changed from 1.8 to 2.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 jburke

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 jburke

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 rcgill

  • Priority changed from undecided to low

Changed 5 years ago by jburke

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

comment:6 Changed 5 years ago by jburke

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 follow-up: Changed 5 years ago by 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).

comment:8 Changed 5 years ago by rcgill

  • Resolution set to fixed
  • Status changed from assigned to closed

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 rcgill

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 rcgill

  • Milestone changed from 2.0 to 1.8
  • Type changed from enhancement to defect
Note: See TracTickets for help on using tickets.