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

Fix TypeError in strict mode #86

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

unreadable
Copy link

Using polka as a handler for firebase cloud functions will cause the following error to be thrown: "TypeError: Cannot set property path of # which has only a getter".

Basically, "strict mode" won't allow changing the request path since it got no setter implemented.
These changes fix it.

In strict mode, the following error will be thrown: "TypeError: Cannot set property path of #<IncomingMessage> which has only a getter", this should fix it.
@unreadable unreadable closed this Mar 3, 2019
@unreadable unreadable reopened this Mar 3, 2019
@ghost
Copy link

ghost commented Mar 3, 2019

this fix breaks polka (see travis) as req.path is used in handling requests for basenames

@unreadable
Copy link
Author

unreadable commented Mar 3, 2019

this fix breaks polka (see travis) as req.path is used in handling requests for basenames

@devcat I see. Will fix it asap.

@ghost
Copy link

ghost commented Mar 3, 2019

that doesn't fix it code requires req.path to be set and that's what you are trying to remove

@codecov-io
Copy link

codecov-io commented Mar 3, 2019

Codecov Report

Merging #86 into master will decrease coverage by 0.95%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
- Coverage     100%   99.04%   -0.96%     
==========================================
  Files           4        4              
  Lines         102      105       +3     
==========================================
+ Hits          102      104       +2     
- Misses          0        1       +1
Impacted Files Coverage Δ
packages/polka/index.js 98.46% <75%> (-1.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 302d74a...79f1864. Read the comment docs.

@unreadable
Copy link
Author

that doesn't fix it code requires req.path to be set and that's what you are trying to remove

@devcat Yeah, figured it out. Tests seem to pass now, will checkout for code coverage as well.

Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks. I don't use Polka in GCP functions because they have a lot of stuff built-in to the platform, so much of Polka (or anything) ends up being redundant.

The use of try/catch & Object.defineProperty is actually detrimental to the execution performance. 😕 Can you change it to my suggestion below? It should still work with the strict-mode getter.

if (!req.path) req.path = info.pathname;

Thanks~!

@unreadable
Copy link
Author

unreadable commented Mar 4, 2019

@lukeed thanks for the sugestion, but unfortunately it doesn't pass the tests. What about removing the try/catch block and using this instead?

Object.defineProperty(req, 'path', {
	writeable: false,
	value: info.pathname
});

@lukeed
Copy link
Owner

lukeed commented Mar 4, 2019

Aye, my bad, that was an oversight. It won't update the req.path as it traverses down into sub-applications or middleware groups (since req.path is now defined).

It's O.dP that is slow – that's what I'm trying to avoid. The try/catch makes it a bit slower because it happens as an else-clause, effectively.

I may have to rethink this a little bit. Does it work correctly within a GCP function with your original changes?

@unreadable
Copy link
Author

@lukeed I see. We're using firebase cloud functions, but I think they are the same as google cloud ones. I managed to make it run using the try/catch + o.dP in the end, also looked to lift the "strict mode" so the error won't be thrown anymore. I'll be looking for other workarounds as well.

@lukeed
Copy link
Owner

lukeed commented Mar 4, 2019

Yeah, they're the same thing :)

Cool – curious, but what's setting strict mode in the first place?

@unreadable
Copy link
Author

@lukeed by inspecting the modules, seems that n_m/firebase-function, which takes my handler, uses strict mode. Haven’t found a way to disable it so far.

@lukeed
Copy link
Owner

lukeed commented Mar 4, 2019

Ah right, yeah. That would make sense.

I'll play with this tonight, but again, I personally don't use Polka inside my GCP functions because it'd be pointless. Express is already "baked in" to the firebase-functions library, so running Polka on top of it just duplicates the work & functionality that's already there and unavoidable.

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 this pull request may close these issues.

None yet

3 participants