React lite theme loader incorrectly resolves paths

[vkammerer]

The new react-lite theme comes with its own loader.js file, but it seems that it incorrectly resolves many paths, as can be seen in the following screenshot:

[joefiorini]

I'm seeing the same issue. Looking into it, it appears some paths (ie. node_modules/react/node_modules/object-assign.js) are incorrectly adding an extension, while others (ie. node_modules/react/cjs/node_modules/object-assign/package.json) are treating cjs as the root, even though it does not contain a package.json.

[bobzhang]

This is actually expected behavior, since the client side has no knowledge about the server side js layout, so it has to try and see the result, does everything work correctly ignoring those logs?

[vkammerer]

I just made a few changes to the loader and created a Pull Request (I redid the commit a few times and just realized that my # mentions were creating noise in this issue -- oops sorry): #2954

It does two things:

• assume that a packaged module's "parent" is the root
• use the package.json "main" property to resolve the id if defined

@bobzhang I don't know if those changes break your implementation, as I have not taken the time to fully understand each specific case, so I don't know if the PR is valid.

I tested it in the boilerplate code that comes with the react-lite theme and it removes all the incorrect calls and the app still works fine.

[vkammerer]

After reading through https://nodejs.org/docs/latest/api/modules.html#modules_all_together and http://npm.github.io/how-npm-works-docs/npm3/how-npm3-works.html I understand now that your implementation is correct and that my Pull Request is incorrect (I will cancel it).

Since npm > 3 resolves paths depending on the "install order", it is impossible to predict where a package is actually located and the only way is to recursively try fetching each file.

I only wish we could find a way that doesn't lead to so many errors in the console..

[vkammerer]

@bobzhang what about having the server inform the client about the packages paths, instead of having the client making guesses and trying requests until it works?
On the server side, we could use require.resolve to locate the packages and send that information down to the client.
This could be done via the websocket connection with two possible strategies:

• 1. on startup, with a node script that would parse the root node_modules directory and create a map of all modules. The advantage of this solution is that it would be efficient and a one time task.The drawback is that if a new package is installed after the startup, the client would have no knowledge of it.
• 2. whenever a module is requested by the client, it could ask the server via the websocket connection to resolve its path before requesting it itself.

[bobzhang]

Note you can hide those logs in chrome devtools. I thought about it, but the added value seems not to justify its complexity which results in less reliability.

Note maybe some of your patch can be employed to reduce such miss hit, for example: use the package.json "main" property to resolve the id if defined

[AdamBrodzinski]

@bobzhang If this behavior will be the accepted norm, you may want to add some kind of warning on the blog/documentation for the package errors. I kind of assumed it was a bug or some kind of work in progress. Also thanks so much for your hard work! It's greatly appreciated! 🍻

Worth noting for future readers... You can filter the noisy logs easily by ignoring all network errors (at the expense of not seeing legitimate errors in the console area):

You could also right click on each error and click "hide" but I think it might hide all errors from that file in the future which could be an issue for the react.js file, etc...

[vkammerer]

Of course thank you for the great work @bobzhang , and the current implementation is a good move to rethink the developer experience, and to get rid of the overhead that webpack used to bring when used with reason.

However, in my opinion, the current errors in the console are not "acceptable" in terms of developer experience, especially since reason has a strong focus in good DX, and even more since this attempt at removing webpack is, from what I understand, also meant to improve DX (by removing one more dependency and relying on an additional node process).

The dev tools console is the main interface developers use to understand what goes on in their app, and I don't think it can be painted all red like this just to accommodate npm's non deterministic installation algorithm...

@AdamBrodzinski : while I understand your recommendation to add notes in the documentation and your tips to filter the logs in the console, I don't see how this could become the norm and something everyone new to reason should be facing when trying it.

@bobzhang maybe the loader can be improved by using the main property in package.json, but such an optimization would not fundamentally solve the issue of those error logs.

[ccapndave]

I think I might have found a problem with the path resolution.

I'm trying to use bs-css (https://github.com/SentiaAnalytics/bs-css) and loader.js tries to load http://localhost:8080/node_modules/glamor/index.js instead of http://localhost:8080/node_modules/glamor/lib/index.js, which causes a runtime error. It looks like the loader isn't respecting the path in main in glamor's package.json (which is lib/index.js). Could this be?

[Et7f3]

We might close this issue because this template isn't available anymore 01abf6e

And maybe open a new one or rename this one because

This happens to delete the loader.js setup in react-lite. I'll port it
over to the react-hooks template later.

from the commit msg.

[stale]

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.