Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

5.2.x breaks express.js route handling logic? #1192

Closed
nlfiedler opened this issue May 24, 2021 · 13 comments
Closed

5.2.x breaks express.js route handling logic? #1192

nlfiedler opened this issue May 24, 2021 · 13 comments

Comments

@nlfiedler
Copy link

The 5.1.0 version of pkg works fine for compiling and running our application, but the 5.2.1 (and maybe 5.2.0) changed in a way that the route handling in Express.js fails with a strange error.

::ffff:127.0.0.1 - - [24/May/2021:21:49:04 +0000] "GET / HTTP/1.1" 500 148
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type number (0)
    at validateString (internal/validators.js:124:11)
    at Object.resolve (path.js:1039:7)
    at Object.process.pkg.path.resolve (pkg/prelude/bootstrap.js:477:25)
    at View.lookup (/snapshot/helix-auth-svc-2021.1.0/node_modules/express/lib/view.js:114:32)
    at new View (/snapshot/helix-auth-svc-2021.1.0/node_modules/express/lib/view.js:94:20)
    at Function.render (/snapshot/helix-auth-svc-2021.1.0/node_modules/express/lib/application.js:570:12)
    at ServerResponse.render (/snapshot/helix-auth-svc-2021.1.0/node_modules/express/lib/response.js:1012:7)
    at /snapshot/helix-auth-svc-2021.1.0/lib/app.js:116:7
    at Layer.handle_error (/snapshot/helix-auth-svc-2021.1.0/node_modules/express/lib/router/layer.js:71:5)
    at trim_prefix (/snapshot/helix-auth-svc-2021.1.0/node_modules/express/lib/router/index.js:315:13)

The code in question is at https://github.com/perforce/helix-authentication-service although I doubt any of that is relevant.

Let me know what helpful information I can provide. I realize that what has been said so far is not much to go on.

@robertsLando
Copy link
Contributor

@jesec ?

@jesec
Copy link
Contributor

jesec commented May 25, 2021

The only significant change in 5.2.x that may be related to this issue is the compression.

Other changes are just routine updates (bump Node.js versions, etc.) or macOS-only (codesigning, M1 support, etc.), which are quite unlikely to be related. #1173 is indeed a global functional change, but I don't think that's related.

The compression patch is quite large, and some elements in it can cause unintended behavioral change to no-compression use case, as we can see in #1189 .

@nlfiedler If possible, can you provide a minimum repository for reproduction of the issue?

cc: @erossignon

@jesec
Copy link
Contributor

jesec commented May 31, 2021

@nlfiedler I still need a minimum repo to reproduce. I can't reproduce it in my own Express.js project.

jesec added a commit that referenced this issue May 31, 2021
…tem (#1115)"

This reverts commit 82ce625.
This reverts commit ea23196.
This reverts commit ea7d72b.

Bug: #1191, #1192
jesec added a commit that referenced this issue May 31, 2021
This reverts commit 82ce625.
This reverts commit ea23196.
This reverts commit ea7d72b.

Bug: #1191, #1192
jesec added a commit that referenced this issue May 31, 2021
This reverts commit 82ce625.
This reverts commit ea23196.
This reverts commit ea7d72b.

Bug: #1191, #1192
@erossignon
Copy link
Contributor

erossignon commented May 31, 2021

@jesec, there was an issue in the compressed virtual file system that causes confusion for file that are dynamically accessed inside the /snapshot, as you spotted out. This is now fixed in #1200.
@nlfiedler I would be please to add a unit test if you describe a simple way to reproduce the behavior you found. so I can ensure that this PR fixes the issue. Alternatively, you could try to fetch #1200 and experiment on your side.

@dabenard
Copy link

dabenard commented May 31, 2021

@erossignon I am not related to @nlfiedler but I am facing what I believe to be the same issue in one of my projects.
You can find a repository containing minimal code to reproduce the issue here: https://github.com/dabenard/pkg-express.

erossignon added a commit to node-opcua/pkg that referenced this issue Jun 1, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 1, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 1, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 1, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 1, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 1, 2021
jesec pushed a commit that referenced this issue Jun 2, 2021
jesec pushed a commit that referenced this issue Jun 2, 2021
jesec pushed a commit that referenced this issue Jun 2, 2021
jesec pushed a commit that referenced this issue Jun 3, 2021
jesec pushed a commit that referenced this issue Jun 3, 2021
jesec pushed a commit that referenced this issue Jun 3, 2021
This reverts commit 52ddf23.

v2:

- #1191
- only compress VFS if --compress is set (#1192)
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 3, 2021
@b-karin
Copy link

b-karin commented Jun 4, 2021

We are successfully using v5.2.0 with one of our Express applications as it only sends json responses.
But with new Express app with SSR on res.render I am facing the same issue.

erossignon added a commit to node-opcua/pkg that referenced this issue Jun 5, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 5, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 7, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 8, 2021
erossignon added a commit to node-opcua/pkg that referenced this issue Jun 8, 2021
jesec pushed a commit that referenced this issue Jun 8, 2021
)

v2:

- fix issue #1191 
- only compress VFS if --compress is set (#1192)
- improve random access with the compressed virtual file system
@robertsLando
Copy link
Contributor

Facing same issue with pkg@5.2.1. @erossignon is this fixed with your last PR? If so @leerob could you make a new release?

@erossignon
Copy link
Contributor

Yes this is fix and awaiting the new release

@robertsLando
Copy link
Contributor

@leerob New release so?

@sebastianrath
Copy link

I have the same question, is there an ETA for the new release? Thanks a lot!

@robertsLando
Copy link
Contributor

@leerob Sorry for pinging you again but we really need a new release here

@leerob
Copy link
Member

leerob commented Jun 24, 2021

Are we ready?

@leerob
Copy link
Member

leerob commented Jun 24, 2021

Released!

eltociear added a commit to eltociear/pkg that referenced this issue Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants