Update to Node 14

[adamretter]

Closes #629

[line-o]

@adamretter this is already part of #626

[line-o]

Interestingly you also chose npm version 6
Can you tell me why npm 7 cannot be used?

[line-o]

good changes but would be conflicting with #626

[adamretter]

@adamretter this is already part of #626

Okay I didn't realise that! Regardless, it should likely be a separate PR otherwise there are mixed concerns... which could explain why I didn't realise it was part of that other PR ;-)

Can you tell me why npm 7 cannot be used?

No idea. I didn't even look at npm 7, because...
If you download Node v14.17.0, then it comes with npm 6.14.13... So I figured it was best to use the recommended versions together.

[line-o]

OK, so it is the bundled version.
While I see a separate PR is the be better way, it is more important to get #626 in.
It contains the changes needed for the release of eXistdb 5.3.0

[adamretter]

@line-o It's simple enough to have both. Merge this and you/I can rebase the other in minutes ;-)

[line-o]

Weird, now the cypress tests are failing in this branch as well. (the sole reason why #626 is not merged yet)

[joewiz]

My inclusion of Node 14 in #626 was just a test to see if CI would pass with it. If we get it working here I'll force push a clean branch.

[adamretter]

Test failures here are different though I think

…

On Thu, 10 Jun 2021, 18:35 Juri Leino, ***@***.***> wrote: Weird, now the cypress tests are failing in this branch as well. (the sole reason why #626 <#626> is not merged yet) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#638 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJUTOKU7AOIUXKEBCQ7LXLTSDSUZANCNFSM46OZADYQ> .

[line-o]

https://github.com/eXist-db/documentation/pull/638/checks?check_run_id=2795137601#step:6:553
was also reported by @joewiz
@duncdrum do you have an idea?

[line-o]

This is likely the update to cypress 7.4 by the looks of things

[joewiz]

And we're green!

From https://docs.cypress.io/api/commands/visit#Syntax:

Cypress will prefix the URL with the baseUrl configured in your network options if you've set one.

Our cypress.json defines baseUrl as http://localhost:8080/exist/apps/doc/, so the cy.visit('.') in documentation_spec.js was causing cypress to request:

http://localhost:8080/exist/apps/doc/.

I wasn't able to force a request to . with curl to see if I could induce eXist to return the content-type: application/xml reported by CI. But, it’s clear that removing the period ensures the request goes to the intended URL:

http://localhost:8080/exist/apps/doc/

Once I made this change, npm run cypress passed locally, so I committed the change to this branch, and it passes here too.

[joewiz]

My hunch is that cypress-io/cypress#8629, which was released in Cypress 5.3.0, somehow affected our changes to baseUrl and the cy:visit call in #519. That PR passed CI, so there must be some other variable(s) here... but I think it's pretty clear that the . isn't necessary given the definition of baseUrl.

[joewiz]

I was able to reproduce the request to http://localhost:8080/exist/apps/doc/. using Postman. eXist returns the collection contents, as if we'd requested http://localhost:8080/exist/rest/db/apps/doc!

In my testing, this is the result of the controller.xql's "ignore" directive on "all other requests." One takeaway for users of eXist's URL rewriting facility is that if you don't want your collection contents to be listable (and resources viewable), you definitely shouldn't use an "ignore" fallback; you should implement an error page of some kind and set an appropriate status code.

[duncdrum]

Ah I remember now this being part of the release notes, I would suggest to move to cy.visit(‘/‘) in a separate future update obviously adjusting the baseUrl in the config.