File paths are OS dependent

[olane]

I'm not sure if you consider this a bug or not, but the config requires system-specific paths in the entries array (i.e. you must use \ on windows and / elsewhere for path separators).

If you don't, you get the rather cryptic error

TypeError: Cannot read property 'contents' of undefined

The solution for me was to use path.join to generate the system-specific path. It might be nicer if the plugin handled this for you, either by using some sort of glob pattern instead of specific file paths, or by translating between / and \ versions. Improving the error and/or documentation might be alternative options.

[ismay]

Hi @olane, thanks for bringing this up. It is indeed a bit of a cryptic error. I think that in this case the user is responsible for supplying the correct paths, that's the case for other metalsmith plugins as well and node apps in general, and I think can reasonably be expected of the user.

I think it would be good to test if the file exists after this line, by using fs.existsSync with filePath. If it doesn't we can throw a more descriptive error. In addition we should also mention this in the readme.

Would you mind opening a PR for that?

[olane]

Yep, happy to have a look when I get the time :)

[ismay]

Thanks!

[ismay]

Hi @der-On, I know that this is originally your plugin, but as a co-maintainer I'm not that happy with what you did here. There is a PR in the works to address this issue that @olane was working on.

You should have discussed your alternate strategy to deal with this issue here, or in the PR before implementing it. That way we could have reached a consensus before implementing and weighed our options. Which is not even mentioning that you're committing code directly to master, which is failing the CI checks, and then publishing that as well. I'm not sure what the idea is here, but I'm not happy with it.

[der-On]

@ismay I'm sorry for beeing so selfish. I have overlooked the PR and even the linter. I've just run the tests locally, which went fine. In fact I've needed the fix while working on a project to make metalsmith-watch work and was in a hurry.

Shall I unpublish and create a reverting commit at master?

[ismay]

Ah ok I see. Well that's ok, can happen of course.

Shall I unpublish and create a reverting commit at master?

I think it's ok to leave it for now. I think your changes also add an important bit of functionality, as we also want people to be able to use metalsmith watch.

I think that it'd be best to allow toggling the throwing of an error when a file is not found. We could throw by default, and allow suppressing it with an option, like so: https://github.com/metalsmith/metalsmith-layouts#suppressnofileserror. What do you think @olane @der-On?

I think that would allow usage of metalsmith-watch and allow a more descriptive error to be thrown by default when a file can't be found.

[olane]

Yep, I agree with being able to configure whether it throws or not (and that it's important to be able to use metalsmith-watch).

Presumably if you're using metalsmith-watch you also need to configure it to rebuild the right files when any of your browserified js is changed - would that mean passing all your js to be rebuilt when a js file changes, or just the entry points? Either way it would be a good addition to the readme.

Perhaps it makes sense to close my PR if the error throwing behaviour is going to be configurable?

[der-On]

@ismay @olane I've used debug to inform about the missing file, instead of throwing an error. This way people can use DEBUG=metalsmith-browserify to be informed about the missing files. I know this is not ideal, but there is no way of a metalsmith plugin to know if it is the initial build or an incremental build triggered by metalsmith-watch. Or maybe we could detect the initial build, by setting an internal build counter and throw during the first build, but not during incremental builds?

[ismay]

I would recommend this approach: https://github.com/metalsmith/metalsmith-layouts/blob/master/lib/index.js#L126

It has worked well for metalsmith-layouts.

So:

1. Throw by default. It should be clear what's going on, without having to use debug.
2. Allow suppressing the error if you're using metalsmith-watch (and log with debug). You don't have to detect usage of metalsmith-watch, it's up to the user to configure it. So if you use metalsmith-watch, you also set suppressBlaBlaError to true.

That addresses both use-cases.

[der-On]

@ismay That sounds absolutely reasonable. Thank you.

[ismay]

Published as 1.1.0. Let me know how it works!