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

Make browser field adjust incoming "deep" module references. #51

Closed
justinbmeyer opened this issue Jan 10, 2015 · 17 comments
Closed

Make browser field adjust incoming "deep" module references. #51

justinbmeyer opened this issue Jan 10, 2015 · 17 comments

Comments

@justinbmeyer
Copy link
Contributor

Related to browserify/browserify#1065 (comment)

I'd like a browser field like:

browser: {
  "./control": "./dist/cjs/control"
}

To be able to adjust a mapping like:

require("can/control") //-> to can/dist/cjs/control

To make this work, one might only have to make paths include the dependency module's path (node_modules/can) if the id is not relative around here: https://github.com/defunctzombie/node-browser-resolve/blob/master/index.js#L200

@justinbmeyer
Copy link
Contributor Author

Well, it would be a little tricker. One would have to probably resolve the "can" part first to know where it's package.json file is.

@defunctzombie
Copy link
Collaborator

In your example, the browser field key is not going to match the module require. We could possibly support a key of can/control but I don't think this is a good route to go down. For third party modules you would either replace the whole module or that module itself needs a package.json field to replace the file of interest.

@justinbmeyer
Copy link
Contributor Author

@defunctzombie Thanks for your response! I'm not sure what you mean by:

For third party modules you would either replace the whole module or that module itself needs a package.json field to replace the file of interest.

I'll try to explain my use case a bit more. I'm attempting to create something that allows someone to write in ES6 and export to AMD / CJS and make them all useful to npm/jspm/SystemJS, npm/browserify and bower/amd.

My target project is CanJS. Like many other libraries / frameworks, it contains many modules itself: can/compute, can/control, etc.

I'd like people to be able to import, require, and define these with the same module ids with minimal configuration possible.

I believe this might be a very common pattern for development. I think Ember is already doing something like this, but not publishing to npm a version useful to JSPM/SystemJS (only useful to browserify which is easier).

I could force developers to add the browser map in every single project like:

    "browser": {
        "can/component/component": "can/dist/cjs/component/component",
        "can/construct/construct": "can/dist/cjs/construct/construct",
        "can/map/map": "can/dist/cjs/map/map",
        "can/list/list": "can/dist/cjs/list/list",
        "can/list/promise/promise": "can/dist/cjs/list/promise/promise",
        "can/observe/observe": "can/dist/cjs/observe/observe",
        "can/compute/compute": "can/dist/cjs/compute/compute",
        "can/model/model": "can/dist/cjs/model/model",
        "can/view/view": "can/dist/cjs/view/view",
        "can/view/ejs/ejs": "can/dist/cjs/view/ejs/ejs",
        "can/view/stache/stache": "can/dist/cjs/view/stache/stache",
        "can/control/control": "can/dist/cjs/control/control",
        "can/route/route": "can/dist/cjs/route/route",
        "can/control/route/route": "can/dist/cjs/control/route/route",
        "can/view/mustache/mustache": "can/dist/cjs/view/mustache/mustache",
        "can/route/pushstate/pushstate": "can/dist/cjs/route/pushstate/pushstate",
        "can/model/queue/queue": "can/dist/cjs/model/queue/queue",
        "can/construct/super/super": "can/dist/cjs/construct/super/super",
        "can/construct/proxy/proxy": "can/dist/cjs/construct/proxy/proxy",
        "can/map/lazy/lazy": "can/dist/cjs/map/lazy/lazy",
        "can/map/delegate/delegate": "can/dist/cjs/map/delegate/delegate",
        "can/map/setter/setter": "can/dist/cjs/map/setter/setter",
        "can/map/attributes/attributes": "can/dist/cjs/map/attributes/attributes",
        "can/map/validations/validations": "can/dist/cjs/map/validations/validations",
        "can/map/backup/backup": "can/dist/cjs/map/backup/backup",
        "can/map/list/list": "can/dist/cjs/map/list/list",
        "can/map/define/define": "can/dist/cjs/map/define/define",
        "can/map/sort/sort": "can/dist/cjs/map/sort/sort",
        "can/control/plugin/plugin": "can/dist/cjs/control/plugin/plugin",
        "can/view/modifiers/modifiers": "can/dist/cjs/view/modifiers/modifiers",
        "can/util/object/object": "can/dist/cjs/util/object/object",
        "can/util/fixture/fixture": "can/dist/cjs/util/fixture/fixture",
        "can/view/bindings/bindings": "can/dist/cjs/view/bindings/bindings",
        "can/view/live/live": "can/dist/cjs/view/live/live",
        "can/view/scope/scope": "can/dist/cjs/view/scope/scope",
        "can/util/string/string": "can/dist/cjs/util/string/string",
        "can/view/autorender/autorender": "can/dist/cjs/view/autorender/autorender",
        "can/util/attr/attr": "can/dist/cjs/util/attr/attr"
    }

... or the browser field might be able to do this mapping for them.

Please let me know if you have other ideas. Thanks again!

@justinbmeyer
Copy link
Contributor Author

One more bit of information ...

I considered having two npm packages ... one for browserify, the other for jspm/SystemJS. This doesn't work because the module names requested would still be different.

require("can-browserified/component");

@justinbmeyer
Copy link
Contributor Author

In your example, the browser field key is not going to match the module require.

Yep, but I could probably make it treat the "can" package's browser field a bit differently. If the module is can/control, it would look for ./control paths in the "can" browser field.

@defunctzombie
Copy link
Collaborator

Just so I understand because it is not clear from the current discussion.

You want users to be able to do the following:

require('can/control');

And in your can module package.json you want to have the following:

browser: {
  "./control": "./dist/cjs/control"
}

So that when the user builds their module, it will use dist/cjs/control for their can/control require. Is that correct?

@justinbmeyer
Copy link
Contributor Author

Yes.

Sent from my iPhone

On Jan 11, 2015, at 11:57 PM, Roman Shtylman [email protected] wrote:

Just so I understand because it is not clear from the current discussion.

You want users to be able to do the following:

require('can/control');
And in your can module package.json you want to have the following:

browser: {
"./control": "./dist/cjs/control"
}
So that when the user builds their module, it will use dist/cjs/control for their can/control require. Is that correct?


Reply to this email directly or view it on GitHub.

@defunctzombie
Copy link
Collaborator

@justinbmeyer I won't have time to fix this myself soon but will review a PR for it if you need the fix before I get to it. I agree that this behavior should be supported.

/cc @substack

@justinbmeyer
Copy link
Contributor Author

sweet! I'll work on it this weekend.

@justinbmeyer
Copy link
Contributor Author

@defunctzombie /cc @substack

I looked into this a bit today. My current approach is that if a module id looks like: "BASE/**", I'm going to add to paths all the node_module/foo paths. So if basedir is at:

root/modue-a/node_modules/module-b/node_modules/module-c/somewhere/

I'll add:

root/node_modules/module-a/node_modules/module-b/node_modules/BASE
root/node_modules/module-a/node_modules/BASE
root/node_modules/BASE

To complicate matters slightly, if basedir is at:

root/dir-a/dir-b/somewhere/

I'll have to walk up until I find a package.json and then add:

root/node_modules/BASE

Finally, I'm going to change find_shims_in_package to include a shim key for the mapped name.

        key = path.resolve(cur_path, key);
        shims[key] = val;

Becomes something like:

        shims[path.join( pkgJson.name, key)] = val;
        key = path.resolve(cur_path, key);
        shims[key] = val;

So if package.json looks like:

{
  "name" : "base",
  "browser: { "./mod-a": "./browser-mod-a" }
}

shims will have "base/mod-a": "path/to/browser-mod-a".

Any thoughts on this approach? I noticed none of the tests set basedir. Is this something that I can add to a test? Thanks!

@defunctzombie
Copy link
Collaborator

wouldn't this be simpler if the underlying resolve module gave you the package.json for the module which contains the file you required?

@justinbmeyer
Copy link
Contributor Author

I thought about that, but I didn't see a necessarily less complex way of doing it. The resolve module doesn't currently read the package.json for ids like "base/path". It simply starts removing node_module folders and checking if there is something there. The alternative you suggest would add:

  • a hook that enabled an alternate package.json location (it looks in base/path/package.json, I would need to configure it to look in base/package.json)
  • a configurable path read from that location given the package info.

It felt less than ideal putting this all in resolve. I figured that resolve should be doing as little as possible to support the browser config.

But, I'm happy to make it work that way. What do you think?

Sent from my iPhone

On Jan 18, 2015, at 2:01 AM, Roman Shtylman [email protected] wrote:

wouldn't this be simpler if the underlying resolve module gave you the package.json for the module which contains the file you required?


Reply to this email directly or view it on GitHub.

@defunctzombie
Copy link
Collaborator

I think it should go in resolve. I ran into this problem before with resolve and the issue is basically that for some require styles (just the module name) you get package.json metadata from resolve and for others you don't. It would be nice if that module was consistent and always returned the metadata for the path it resolved that way anyone using that module could leverage having the module metadata for the module owning the file. This has nothing to do with being browser config specific.

@justinbmeyer
Copy link
Contributor Author

@defunctzombie This will change existing test results. For example:

https://github.com/justinbmeyer/node-resolve/blob/master/test/resolver.js#L12

Instead of undefined, will return node-resolve's package.json: https://github.com/defunctzombie/node-browser-resolve/blob/master/package.json because it is the first package.json in any parent folder.

Is this ok?

Also, should I move this discussion into node-resolve?

@defunctzombie
Copy link
Collaborator

We don't want node-resolves package.json tho. We want the package.json for the module which contains the file being resolved.

Yes, I would bring up this discussion in node-resolve as an enhancement and link to this issue. I am pretty sure that node-resolve should be updated to support this behavior.

@justinbmeyer
Copy link
Contributor Author

We don't want node-resolves package.json tho. We want the package.json for the module which contains the file being resolved.

Yes, but in this case, it would be node-resolves package.json right? It is the module that contains test/resolver/foo.js.

@justinbmeyer
Copy link
Contributor Author

Now that browserify/resolve#63 landed, I'll work on making node-browser-resolve work against node-resolve 1.1.0. I should have something by tonight. Thanks!

justinbmeyer added a commit to justinbmeyer/node-browser-resolve that referenced this issue Feb 4, 2015
justinbmeyer added a commit to justinbmeyer/node-browser-resolve that referenced this issue Feb 5, 2015
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

No branches or pull requests

2 participants