v1.5.0 requires additional Webpack config

[Josh-Cena]

Just jumping in to say that, with my current setup, v1.4.0 worked fine without any changes, but updating to v1.5.0 required me to add the following to my webpack config:

node: {
  global: true
},
resolve: {
  fallback: {
    stream: 'stream-browserify'
  }
},
plugins: [
  new webpack.ProvidePlugin({
    process: 'process/browser',
    Buffer: ['buffer', 'Buffer']
  })
]

and had to install stream-browserify.

The problematic line was const Transform = require('stream').Transform, but this line is also present in v1.4.0. This is a node core package, so it is expected for some kind of polyfill to be needed, but for some strange reason, v1.4.0 just works without any particular configuration change. 🤷‍♂️

Originally posted by @miguelcobain in #38 (comment)

[Josh-Cena]

Hi @miguelcobain I created a new issue for you.

I actually don't know much about your use-case. Are you trying to use Buffer in a browser environment? (I don't think people use Webpack for Node apps?) What was the error before you fixed your Webpack?

[miguelcobain]

Yes, I'm using reading-time in a browser context. It works great!

Before my webpack fixes, the error I got was basically complaining about the stream import (which of course doesn't exist in a browser environment).

Here is the error:

ERROR in ./node_modules/reading-time/lib/stream.js 7:63-90
Module not found: Error: Can't resolve 'stream' in '/Users/miguel/workspace/my-project/node_modules/reading-time/lib'
Did you mean './stream'?
Requests that should resolve in the current directory need to start with './'.
Requests that start with a name are treated as module requests and resolve within module directories (node_modules).
If changing the source code is not an option there is also a resolve options called 'preferRelative' which tries to resolve these kind of requests in the current directory too.

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "stream": require.resolve("stream-browserify") }'
	- install 'stream-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "stream": false }

[miguelcobain]

Ok, here's a wild guess. I've been looking at the diff between 1.4.0 and 1.5.0 and I saw this change:

Perhaps webpack wasn't even bundling the stream file at all before, because it wasn't referenced in the index.js file. So perhaps I didn't get an error because I'm not using the stream support in my project.

But now, the stream file is imported, which forces webpack to bundle it, and then this problem emerges.

[Josh-Cena]

Perhaps webpack wasn't even bundling the stream file at all before, because it wasn't referenced in the index.js file. So perhaps I didn't get an error because I'm not using the stream support in my project.

Yeah, that's my guess. So you're just using the normal readingTime but now you're penalized by the additional export. I don't know if we should do anything for that considering we are primarily a Node lib, but you can import from the submodule reading-time/lib/reading-time

[miguelcobain]

Yes! Using import readingTime from 'reading-time/lib/reading-time'; with 1.5.0 worked fine again, without any webpack config changes. Thank you.

Would a quick note about this on the README be useful?

[Josh-Cena]

Would a quick note about this on the README be useful?

Actually I would try using the browser field: https://docs.npmjs.com/cli/v7/configuring-npm/package-json#browser

Can you do me a favor by hacking your node_modules and add "browser: "lib/reading-time.js" in reading-time/package.json? This way you can go back to import readingTime from 'reading-time';. I believe Webpack picks this field up.

[Josh-Cena]

And yes, I would do a quick note. Thanks for the reminder :P

[miguelcobain]

Indeed, adding "browser": "lib/reading-time.js" to reading-time's package.json, and then using import readingTime from 'reading-time';, works great.

But doesn't this mean that browser users will not be able to get the stream feature at all?

[Josh-Cena]

But doesn't this mean that browser users will not be able to get the stream feature at all?

They still can by importing from lib/stream, but the stream version is rarely used anyways, and with an even much smaller group who would use it in a browser context. Before 1.5.0 it wasn't even exported in the main entry point, so... let's make the majority happy

[miguelcobain]

I can agree with that. It's definitely more in line with past versions and less of a breaking change!

[Josh-Cena]

I added the fix in e34abfe (too lazy to send a PR). And you can read the README to see if that seems good :D

[Josh-Cena]

Unfortunately we are already shipping 2.0.0, so not sure if this should be backported to v1... @ngryman Do you think we should do a patch release in v1, or should we encourage people to pick up on v2?

[miguelcobain]

Perhaps adding the note that you mentioned above:

They still can by importing from lib/stream

Bonus points if we could add the necessary webpack config I posted in a spoiler like this: https://gist.github.com/jbsulli/03df3cdce94ee97937ebda0ffef28287

But, to be honest, this is already excellent. :)

[Josh-Cena]

Perhaps adding the note that you mentioned above:

I did that in a later commit, so just read the docs on the front page instead of looking at the diff.

Bonus points if we could add the necessary webpack config I posted in a spoiler like this:

I suspect users who need to use the stream version already know about the Buffer polyfill, so... maybe not too much education ;)

[Josh-Cena]

We'll keep the issue open until @ngryman and I agreed on a release method. Thanks for reporting @miguelcobain!

[miguelcobain]

It's perfect!
My pleasure. Thanks for the quick turnaround and useful library.

[ngryman]

I just caught up, thanks for taking care of this @Josh-Cena 🙏

I just published 2.0.0-1 with your latest fix: https://github.com/ngryman/reading-time/releases/tag/v2.0.0-1.
I also re-organized the docs a bit for the browser's usage: https://github.com/ngryman/reading-time#usage.

@Josh-Cena Unfortunately we are already shipping 2.0.0, so not sure if this should be backported to v1... @ngryman Do you think we should do a patch release in v1, or should we encourage people to pick up on v2?

I think it's fine if we don't backport it. Let's see if we get other reports about this. There's an easy workaround anyway, as @miguelcobain suggested: import readingTime from 'reading-time/lib/reading-time'.

@Josh-Cena We'll keep the issue open until @ngryman and I agreed on a release method. Thanks for reporting @miguelcobain!

Thanks for the help guys!