search

zooniverse / shakespeares_world

Full text transcription project for the Folger Shakespeare Library
https://www.shakespearesworld.org
Other
8 stars 5 forks source link

Upgrade panoptes-client #367

Closed simoneduca closed 6 years ago

simoneduca commented 6 years ago

Todos

eatyourgreens commented 6 years ago

Is that the Docker build and deploy that's failing?

simoneduca commented 6 years ago

AFA I can see, it's the gulpbrowserify task in the build that's broken. For ref https://github.com/terinjokes/gulp-uglify/issues/249#issuecomment-262406528

simoneduca commented 6 years ago

I thought I had seen this before https://github.com/zooniverse/shakespeares_world/pull/349#pullrequestreview-78874986

eatyourgreens commented 6 years ago

npm run build runs without error for me, locally. :man_shrugging:

Oh wait, I didn't wait for it to finish. Error city. 😢

eatyourgreens commented 6 years ago

The Panoptes client went through a pretty big jump here, 2.2 -> 2.9. Did one of those version changes introduce JavaScript that the gulp task can't parse? I have no idea.

simoneduca commented 6 years ago

But it fails also without the client upgrade. The mystery is that yesterday I published some changes with no problem.

camallen commented 6 years ago

It builds ok for on v8.9.3 for me.

simoneduca commented 6 years ago

If I use pump like I had suggested, it build fine in node 9.2. PR coming with that fix.

simoneduca commented 6 years ago

https://github.com/terinjokes/gulp-uglify/tree/master/docs/why-use-pump

eatyourgreens commented 6 years ago

Node 8.8 fails for me with the same error.

Does pump fix it, or just swallow the error and build a broken JS file?

simoneduca commented 6 years ago

Pump is supposed to allow any error to propagate, unlike pipe. So it should do the opposite of swallowing. We can test with staging the branch though.

simoneduca commented 6 years ago

Here's the pump PR https://github.com/zooniverse/shakespeares_world/pull/368

eatyourgreens commented 6 years ago

Fresh install, with node 9, builds for me without errors. I'll try this branch and report back.

eatyourgreens commented 6 years ago

master builds without problems. This branch fails. I don't know much about gulp or browserify, so I'm afraid I can't be much help here.

camallen commented 6 years ago

confirming master just built for me on v9.4, after a git pull && npm install. though npm install did update the package-lock.json as well.

simoneduca commented 6 years ago

I don't know what was going on, but after a fresh reinstall of the project, I've managed to build and publish this branch. Mind having a look? https://preview.zooniverse.org/shakespearesworld/#!/

eatyourgreens commented 6 years ago

Did it build js/main.js or is preview using the JS file that's there from the last successful build?

simoneduca commented 6 years ago

I wasn't sure. So I did again and, yes, it did build it. Btw this isn't the branch using pump.

eatyourgreens commented 6 years ago

👍🏼 I can have a look tomorrow on a browser that has 3rd party cookies disabled.

simoneduca commented 6 years ago

Thanks! I tested disabling 3rd party cookies on Chrome and login works fine here. I was surprised the login persisted after ending the session though (closing tab). I thought session storage is not persisted in that case.

eatyourgreens commented 6 years ago

https://preview.zooniverse.org/shakespearesworld/js/main.js is an old version of the JS file, without the new client.

I still get GulpUglifyError: unable to minify JavaScript when I try to build this branch locally, after deleting node_modules and running a fresh npm install.

eatyourgreens commented 6 years ago

@simoneduca when I run the build, or the build and deploy tasks, the build/ directory isn't cleaned up after they run, even if the build failed. Are you, perhaps, deploying an old JS file from your machine, even though the build is actually failing?

I've also noticed that the build and deploy completes without error even if js/main.js isn't actually built.

simoneduca commented 6 years ago

@eatyourgreens I can see "panoptes-client":"^2.9.0" in the link you pasted above. Isn't that latest version? My build dir is cleaned every time. This is weird...

simoneduca commented 6 years ago

Could you do me a favour and try cloning a fresh copy of the project?

eatyourgreens commented 6 years ago

The client bundled in js/main.js on preview isn't 2.9. I checked this by searching through the source map for the oauth code in the Chrome dev console.

npm run build works with version 2.8.1 of the client, but not 2.9. I can't see what is tripping up uglify though.

eatyourgreens commented 6 years ago

I am working with a completely fresh copy of SW.

eatyourgreens commented 6 years ago

Adding ES6 support to uglify fixes the build.

eatyourgreens commented 6 years ago

For future reference, I had a completely clean copy of SW. Between builds, I ran this to check that the JS file was being built:

rm -rf build
rm -rf node_modules
npm install
npm run build
simoneduca commented 6 years ago

Ack! Sounds obvious now, thanks! I'll stage the brach.

eatyourgreens commented 6 years ago

@eatyourgreens I can see "panoptes-client":"^2.9.0" in the link you pasted above. Isn't that latest version? My build dir is cleaned every time. This is weird...

I think you might have been looking at a copy that I had deployed.

eatyourgreens commented 6 years ago

The Uglify error was "Name expected", but I can't see what changed in the client JS that would break uglify. However, the version of uglify that SW was using can't process ES6 files.

simoneduca commented 6 years ago

Maybe, but I was looking at the preview before you push the es6 support code. Does it need staging now?

eatyourgreens commented 6 years ago

I've just run npm run deploy-staging again, from this branch. It would be handy if the deploy reported which files it was uploading and overwriting, like publisssh does. Is that something you can change?

simoneduca commented 6 years ago

I can certainly try, but in a different PR perhaps? I've tested the staged branch and it looks fine. Is this ready to go then?

simoneduca commented 6 years ago

This line in client was breaking uglify. Adding ES6 support would let the code through, but older browsers would break. Thanks @eatyourgreens! So we're changing that line on the client, publish the patch and upgrading SW here.

eatyourgreens commented 6 years ago

I'm seeing this now, when I deploy to staging, which I wasn't seeing before so that's good.

[14:13:39] Uploading files to zooniverse-static/preview.zooniverse.org/shakespearesworld/...
[14:13:59] Uploaded build/js/main.js.gz
[14:14:09] Uploaded build/js/main.js
[14:14:28] Uploaded build/js/main.js.map.gz
eatyourgreens commented 6 years ago

@simoneduca your link doesn't link to the broken code, now that it's been fixed.

simoneduca commented 6 years ago

I'll fix the link, but I'm getting a fresh install because I was still seeing the same error after pulling the latest on this brach.

simoneduca commented 6 years ago

Looks good now. I can see the correct client version in main.js and works fine on preview. Merging.

simoneduca commented 6 years ago

I'm also deploying manually to prod, so we can test.