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

Remove "browser" from package.json to fix Browserify #2696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nolanlawson
Copy link

I tried to follow the discussion from #2525, but wasn't really understanding why this was added in the first place.

Using Browserify 11.1.0, it's impossible to use less @2.5.1 as part of your build. I can reproduce it as easily as:

npm install less
echo "require('less')" > index.js
browserify index.js

You'll see the error:

Error: Cannot find module './utils' from '/private/tmp/foo/node_modules/less/dist'
    at /Users/nolan/.npm-packages/lib/node_modules/browserify/node_modules/resolve/lib/async.js:55:21
    at load (/Users/nolan/.npm-packages/lib/node_modules/browserify/node_modules/resolve/lib/async.js:69:43)
    at onex (/Users/nolan/.npm-packages/lib/node_modules/browserify/node_modules/resolve/lib/async.js:92:31)
    at /Users/nolan/.npm-packages/lib/node_modules/browserify/node_modules/resolve/lib/async.js:22:47
    at FSReqWrap.oncomplete (fs.js:82:15)

This commit fixes that.

@SomMeri
Copy link
Member

SomMeri commented Nov 30, 2015

I tested this and can confirm browserify problem, it throws slightly different error on 2.5.3 through.

The reason is that less.js in dist directory was already browserified in grunt.js. Browserify can not run twice over the same code, because the meaning of require function changes after the first run.

  • Original code: require("./path/code") means that there is directory path and file code.js on the filesystem. Code.js file is supposed to contain needed javascript code.
  • Browserified code is supposed to have all dependencies inside bundled lookup table. require("./path/code") means that there is ./path/code entry inside that bundled lookup table and contains whole ./path/code.js file.

When you run browserify again on the same code, it must fail. Already browserified code is located in different directory then original file and filesystem does not contain path/code.js file. In case of less.js 2.5.1 , there is no utils.js file in dist directory. In case of newest less.js 2.5.3, there is no add-default-options.js file.

If we want to have brower property in package.js, it needs to be configured to do the same thing as grunt.js is doing.

@SomMeri
Copy link
Member

SomMeri commented Nov 30, 2015

The other pull request suggests that user should use the noParse option for browserify to overcome this problem. I tried it from command line and did not managed to make it work. I think that we should either document how to make it run or (preferred) merge in this pull request.

@nolanlawson
Copy link
Author

For a more brute-force solution to the require() problem, you could also just run the browserified code through derequire: https://www.npmjs.com/package/derequire

@SomMeri
Copy link
Member

SomMeri commented Dec 1, 2015

We could also link minimized version, that does not have require function. However, that feels hacky to me.

@yoaquim
Copy link

yoaquim commented Jun 23, 2016

@nolanlawson Having the same issue, but removing "browser": "./dist/less.js" did nothing for me; still get the same error. Am I missing something?

@matthew-dean
Copy link
Member

You don't need to Browserify Less, so I don't get this.

@stale
Copy link

stale bot commented Feb 7, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 7, 2018
@seven-phases-max
Copy link
Member

seven-phases-max commented Feb 7, 2018

@matthew-dean

You don't need to Browserify Less ...

Because of less.js? But what if your need just a clean compiler w/o any browser-specific bloatware?
(It's not that I recommend to merge this or not - I rarely use Browserify and/or less.js myself - thus it's not me to review the change. So this is just a remark).

@stale stale bot removed the stale label Feb 7, 2018
@matthew-dean
Copy link
Member

@seven-phases-max AFAIK, the point of Browserify is to take a JavaScript module designed for Node.js, and package it for the browser.

But what if your need just a clean compiler w/o any browser-specific bloatware?

That's somewhat fair if that's the reason, but I think that's approaching the problem from the wrong direction.

That is, while I understand Less.js's historical implementation of generating HTML/CSS on errors automatically on browsers, and automatically parsing stylesheets, I don't think there's any need for that in modern web development. Less should be included as a script, and then an API call should be made to actually grab stylesheets from the page and convert/inject them as CSS. That would be how it would be done today.

So, if the problem is: "I need a browser-based version of Less," that exists.

If the problem is: "I need a browser-based version of Less that doesn't do X," then this is the wrong approach.

Re-reading though, it seems like the problem is that Less is being bundled by Browserify as part of someone's build process, a problem which we somewhat created by adding a "browser" property? 🤔

If we want to have brower property in package.js, it needs to be configured to do the same thing as grunt.js is doing.

@SomMeri Does this PR fix that? I don't know enough about the underlying ecosystem (Browserify) to know if this puts duct tape over the problem or solves it correctly.

@stale
Copy link

stale bot commented Jun 7, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants