bsansouci / reprocessing-example

79 stars 27 forks source link

Firefox 58.0.1 64 bit Ubuntu 16.04 64 bit #3

Closed raould closed 6 years ago

raould commented 6 years ago

might be good to have more explanation of how to get it working in FF and Chrome in addition to Safari?

raould commented 6 years ago

I tried with chromium-browser --disable-web-security and still no luck.

require_polyfill.js:67 [Deprecation] Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.
loadScript @ require_polyfill.js:67
require_polyfill.js Failed to load resource: the server responded with a status of 404 (File not found)
loadScript @ require_polyfill.js:68
localhost/:48 Uncaught SyntaxError: Unexpected token <
    at eval (<anonymous>)
    at loadScript (http://localhost:8000/require_polyfill.js:126:10)
    at require (lib/js/src/index.js:36:18)
    at eval (lib/js/src/index.js:51:26)
    at loadScript (http://localhost:8000/require_polyfill.js:126:32)
    at http://localhost:8000/require_polyfill.js:129:1
bsansouci commented 6 years ago

Could you open the JS console and paste any error here? This seems a bit strange

zploskey commented 6 years ago

I see an identical error on Chromium 63 on Fedora 27. What @raould posted is the entire output in the JS console. It displays a blank page.

zploskey commented 6 years ago

The python server displays the following:

~/src/reprocessing-example$ python -m SimpleHTTPServer
Serving HTTP on 0.0.0.0 port 8000 ...
127.0.0.1 - - [14/Mar/2018 22:53:27] "GET / HTTP/1.1" 200 -
127.0.0.1 - - [14/Mar/2018 22:53:27] "GET /require_polyfill.js HTTP/1.1" 200 -
127.0.0.1 - - [14/Mar/2018 22:53:28] "GET /lib/js/src/index.js HTTP/1.1" 200 -
127.0.0.1 - - [14/Mar/2018 22:53:28] code 404, message File not found
127.0.0.1 - - [14/Mar/2018 22:53:28] "GET /node_modules/Reprocessing/lib/js/src/Reprocessing.js HTTP/1.1" 404 -
127.0.0.1 - - [14/Mar/2018 22:53:28] code 404, message File not found
127.0.0.1 - - [14/Mar/2018 22:53:28] "GET /favicon.ico HTTP/1.1" 404 -

It's trying to load from node_modules/Reprocessing when it should be node_modules/reprocessing. The filesystem on Mac OS is not case sensitive by default, right?

Schmavery commented 6 years ago

@zploskey @bsansouci shoot, we must have missed a spot. Thanks for the help with diagnosis and we'll see if we can find that rogue mis-capitalized string.

Schmavery commented 6 years ago

@zploskey @raould It's probably this one, but I need to test before I can be sure (feel free to edit locally and let us know 😄 ) https://github.com/Schmavery/reprocessing/blob/master/package.json#L2

zploskey commented 6 years ago

That doesn't seem to be it. This repo already uses reprocessing master with that name changed to lower case. The imports in the generated js look like this:

var Reprocessing       = require("Reprocessing/lib/js/src/Reprocessing.js");

and so on for the other imported reprocessing modules.

Lower-casing the name in bsconfig fixed it after doing a clean build. I made a pull request here: Schmavery/reprocessing#70.

zploskey commented 6 years ago

It now fails with another 404:

127.0.0.1 - - [15/Mar/2018 16:18:13] code 404, message File not found
127.0.0.1 - - [15/Mar/2018 16:18:13] "GET /node_modules/reasongl/lib/js/src/RGLConstants.js HTTP/1.1" 404 -
zploskey commented 6 years ago

For one thing node_modules/reasongl doesn't exist.

zploskey commented 6 years ago

Oh, but after blowing away the lock file and reinstalling, node_modules/Reasongl does exist. So it's another case error, this time either in the reasongl repo where the package.json has "name": "Reasongl" or in the reprocessing package.json where it names the dependency "Reasongl" upper-cased. I'm not exactly sure which one determines the directory name in node_modules.

bsansouci commented 6 years ago

Which one decides the folder name depends on the package manager. Yarn will use the name in the project’s package.json, npm5 will use the dep’s package.json’s name and npm4 the same as yarn.

Anyway, we should capitalize everything I think

zploskey commented 6 years ago

So are you saying the package name for Reprocessing should be capitalized in its package.json as well? It looks like it was just lower-cased in January. It does seem to be important that the name in bsconfig and the package name match, including case.

Schmavery commented 6 years ago

Ben and I accidentally weren't on the same page for a while regarding capitalization. We discussed and decided to do capitalized everywhere.

vkammerer commented 6 years ago

It seems that a capitalized name is not authorized in package.json: https://docs.npmjs.com/getting-started/using-a-package.json#requirements

The Bucklescript docs explain that the name should be the same in package.json and bsconfig.json: https://bucklescript.github.io/docs/en/build-configuration.html

the bsconfig.json name should be the same as the package.json name, to avoid confusing corner-cases

So it seems that a lowercase name should be used everywhere instead?

bsansouci commented 6 years ago

Am I the only one here not understanding how this can matter? As long as npm is not inconsistent regarding the folder names it creates, there is no problem right? In case-sensitive file systems the name of the folder will be Reprocessing and the module name Reprocessing, so it'll work because the folder exists. In case-insensitive file systems the casing of the folder name doesn't matter...

Can someone shed some light on what is the edge case that I'm not seeing here?

btw this sounds similar to the argument I was given about naming files with uppercase. I was told that it's bad because git doesn't handle those "well". Well it's not that git doesn't handle files with uppercase, it's that if you name a file with upper/lower case, check it in, and then want to change the casing you're in a bit fucked. Also if some tooling assumes uppercase, and you name your files lowercase, then it'll work on case-insensitive OSs but not on the others. That's what you want to avoid. It doesn't matter which convention you choose. And we chose uppercase because it's slightly clearer for newcomers that MyFile.re is MyFile as a module.

(correct me if I'm wrong!)

bsansouci commented 6 years ago

cc @bobzhang, @jordwalke

vkammerer commented 6 years ago

@bsansouci I personally haven't encountered any bug with it and I'm working on MacOS so it isn't really an issue for me :) It's just that I'm used to working with npm packages all the time and it's the first time I see one that is Capitalized.

But when you say

It doesn't matter which convention you choose

Then wouldn't it be better to use the convention that has been decided by npm, rather than the one that is decided for this package specifically?

zploskey commented 6 years ago

Surveying some of the issues filed on this for NPM, they don't currently enforce lower case names but they may at some point in the future. The convention is lower case and recommended in the docs, which is why I initially argued for lower-casing everywhere. I also notice that when editing the package.json files for the packages with upper-cased names in VSCode, which apparently is running a linter, I get a warning on the name field:

String does not match the pattern of "^(?:@[a-z0-9-~][a-z0-9-._~]*/)?[a-z0-9-~][a-z0-9-._~]*$".

While it's kind of troublesome to keep changing this around, I think we avoid some problems by just going with the flow and making it lower case.