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

chore(docs): better example in README. #2

Merged
merged 2 commits into from
Jun 1, 2017

Conversation

joaovieira
Copy link
Contributor

  1. Without node_modules lots of requires will fail (assuming user is using npm for some or most of the modules).
  2. There are npm modules that don't ignore and include bower.json file, so the loader would use the bower.json incorrectly for certain npm modules.
  3. Give priority to browser field before main.

1. Without `node_modules` lots of requires will fail (assuming user is using npm for some or most of the modules). 
2. There are npm modules that don't ignore and include `bower.json` file, so the loader would use the bower.json incorrectly for certain npm modules. 
3. Give priority to `browser` field before `main`.
README.md Outdated
descriptionFiles: ['bower.json', 'package.json'],
mainFields: ['main', 'browser']
modules: ['bower_components', 'node_modules'],
descriptionFiles: ['.bower.json', 'package.json'],
Copy link
Member

Choose a reason for hiding this comment

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

I guess you mean 'bower.json' here. See the dot prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's really .bower.json :) .bower.json is the metadata file added by Bower when a bower component is installed. Therefore, it does not exist on installed npm packages. This is also the suggested descriptor as seen here.

Some (isomorphic) npm packages do not npmignore bower.json and it's included in the installed package. Thus, webpack finds the node_module, but uses the bower.json file descriptor. Same applies for bower components that include the package.json. For this, it cannot be:

modules: ['bower_components', 'node_modules'],
descriptionFiles: ['package.json', 'bower.json']
// will use bower_components/**/package.json if available

or

modules: ['node_modules', 'bower_components'],
descriptionFiles: ['bower.json', 'package.json']
// will use node_modules/**/bower.json if available

Copy link
Member

Choose a reason for hiding this comment

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

Well it looks like .bower.json is auto generated once package is installed. I have tested with .bower.json descriptor and it starts breaking files resolving for me. The main reason is even bower.json does not exist .bower.json is being generated without any usefull information. For example ded/script.js package is being installed through bower but has only package.json , if I do include .bower.json as a file descriptor it's being resolved incorrectly because .bower.json file always exists.

I would suggest to stick with bower.json for now, atleast until there is a way to skip description file, if "main" attribute is not being found in it.

Copy link
Contributor Author

@joaovieira joaovieira Mar 28, 2017

Choose a reason for hiding this comment

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

Hm, that does not sound right... If you're using the package.json descriptor, then I'd suggest using the npm package instead (https://www.npmjs.com/package/scriptjs). To register in Bower it should've had a bower.json at the root as mentioned in their docs. Somehow they made it, but it does not sound it's correctly configured, as a Bower component must always have the bower.json manifest. In fact you don't need package.json for a bower-only component. .bower.json extends the manifest, the same way npm extends the package.json with installation information. The fact that it only exists for installed packages is the reason it's preferred. Do you have more dependencies like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I don't really like the way webpack handles it now as well. I never wrote any webpack plugin or used enhance-resolve, but I can have a look. Can you keep the PR open in the meantime? It may help others with the same problem.

Copy link
Member

Choose a reason for hiding this comment

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

We can merge, just I would prefer to keep bower.json without a dot for a while if you don't mind, as it works in most cases now.

enhanced-resolve is being exclusively used now as a resolve engine for webpack 2.x.

p.s. a bit unrelated but see the issue . If you could figure out the way on how or it's possible to resolve multiple files per one webpack require, it would be nice to have aswell. It's more of the bower thing, as it tends to include css files in the main attribute aswell, so we wouldn't need to load them separately (e.g. select2, bootstrap, etc..) .

Please share your thoughts or ideas if you are able to find anything. :-)

Thanks!

Copy link
Contributor Author

@joaovieira joaovieira Mar 28, 2017

Choose a reason for hiding this comment

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

Sure, I will.

The problem of keeping bower.json is the same. Some libraries (e.g. debug) add bower.json to the node_modules/debug folder (in other words, don't ignore it). However, because it was installed via npm and not bower, the main file defined in the bower.json (dist/debug.js) does not exist because dist files are ignored. Package.json does have a browser field pointing to a valid file, which is what would allow to use the npm module in the browser, but the loader doesn't even get there, because it uses the bower descriptor. If this makes sense.

I will open a PR for debug to ignore bower.json anyway :)

Cheers

Copy link
Member

@linaspasv linaspasv Mar 28, 2017

Choose a reason for hiding this comment

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

But does it matter for us in this case if bower.json or package.json descriptor file will be used, even if it was installed through NPM. In both cases it should resolve to ./src/browser.js file anyway.

e.g. bower.json file content once package is being installed through npm

{
  "name": "visionmedia-debug",
  "main": "./src/browser.js",
  "homepage": "https://github.com/visionmedia/debug",
  "authors": [

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually fixed in the latest version (not the most commonly used though). Still, I'd like to use package.json for node_modules, bower.json for bower_components, and webpack doesn't make it easy 😬

@linaspasv
Copy link
Member

Hi @joaovieira , thank you for PR! . Everything looks fine, just see my comment.

@linaspasv linaspasv merged commit 83fd243 into codewizz:master Jun 1, 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.

2 participants