Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for npm namespaces #143

Merged
merged 4 commits into from
Aug 1, 2017
Merged

Conversation

rorticus
Copy link
Contributor

@rorticus rorticus commented Jul 31, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

Adding support for npm namespaces when loading modules. Previously, a configuration like this,

require.config({
    packages: [
        {
            name: '@some-package/some-module',
            location: '/path/to/some-package/some-module'
        }
    ]
});

@some-package/some-module would get interpreted as @some-package/some-module.js because package/main syntax is supported. This would cause the package configuration to not apply as the name would not be what you expected.

With this change, if a module id contains the @ sign, it is considered part of the package name.

Resolves #143

@rorticus rorticus requested a review from kitsonk July 31, 2017 17:41
@codecov
Copy link

codecov bot commented Jul 31, 2017

Codecov Report

Merging #143 into master will increase coverage by 0.2%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #143     +/-   ##
=========================================
+ Coverage   85.87%   86.08%   +0.2%     
=========================================
  Files           1        1             
  Lines         545      546      +1     
  Branches      135      133      -2     
=========================================
+ Hits          468      470      +2     
  Misses         32       32             
+ Partials       45       44      -1
Impacted Files Coverage Δ
src/loader.ts 86.08% <71.42%> (+0.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13adb56...d87e593. Read the comment docs.

@agubler
Copy link
Member

agubler commented Aug 1, 2017

@rorticus which issue does this resolve?

@agubler
Copy link
Member

agubler commented Aug 1, 2017

The linked one is this PR :)

@matt-gadd
Copy link
Contributor

@rorticus presumably this doesn't break support for having all packages under one pseudo package, which I think we get as a side effect currently? https://github.com/dojo/widget-core/blob/master/tests/intern.ts#L60

src/loader.ts Outdated
@@ -544,7 +544,7 @@ declare const Packages: {} | undefined;
}

function getModuleInformation(moduleId: string, referenceModule?: DojoLoader.Module): DojoLoader.Module {
let match = moduleId.match(/^([^\/]+)(\/(.+))?$/);
let match = moduleId.match(/^((?:@[^\/]*\/)?[^\/]+)(\/(.+))?$/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you were fixing this, but I think we should clean this regex up.

It seems like the second capture group isn't used and should be ?: out.

regexper

Also, we should use destructuring to be more modern in the way we deal with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kitsonk so I actually ended up taking the regex out..

@rorticus
Copy link
Contributor Author

rorticus commented Aug 1, 2017

OK, this has been updated to support @matt-gadd 's example he mentioned above. The way it works now is that it searches all configured packages and finds the longest match, then uses that to load the module, assuming that the rest of the unmatched package is the module id.

Example,

require.config({
    packages: [
        { name: '@dojo', location: 'node_modules/@dojo' },
        { name: '@dojo/shim', location: '/path/to/shim' },
    ]
});

require([
    '@dojo/shim', // will load from /path/to/shim/main
    '@dojo/shim/Map', // will load from /path/to/shim/Map
    '@dojo/has/has' // will load from node_modules/@dojo/has/has
], (shim, has ) => { /*...*/ })

@rorticus rorticus merged commit 48a4695 into dojo:master Aug 1, 2017
@rorticus rorticus deleted the issue-134 branch August 1, 2017 17:29
@dylans dylans added this to the 2017.08 milestone Aug 9, 2017
@rorticus rorticus mentioned this pull request Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants