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

Firefox 58.0.1 64 bit Ubuntu 16.04 64 bit #3

Closed
raould opened this issue Feb 10, 2018 · 18 comments · Fixed by #6
Closed

Firefox 58.0.1 64 bit Ubuntu 16.04 64 bit #3

raould opened this issue Feb 10, 2018 · 18 comments · Fixed by #6

Comments

@raould
Copy link

raould commented Feb 10, 2018

  • npm run build:web worked, although it did generate a warning about possibly unsafe polymorphic comparison use.
  • then python -m SimpleHTTPServer
  • opened localhost:8000 in Firefox and got a blank page
  • opened index.html directly in Firefox and got a blank page

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

@raould
Copy link
Author

raould commented Feb 10, 2018

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
Copy link
Owner

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

@zploskey
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

For one thing node_modules/reasongl doesn't exist.

@zploskey
Copy link
Contributor

zploskey commented Mar 15, 2018

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
Copy link
Owner

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
Copy link
Contributor

zploskey commented Mar 16, 2018

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
Copy link
Collaborator

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

@vkammerer
Copy link
Contributor

vkammerer commented May 7, 2018

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
Copy link
Owner

bsansouci commented May 7, 2018

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
Copy link
Owner

cc @bobzhang, @jordwalke

@vkammerer
Copy link
Contributor

vkammerer commented May 7, 2018

@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
Copy link
Contributor

zploskey commented May 7, 2018

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.

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

Successfully merging a pull request may close this issue.

5 participants