sveltejs / sapper

The next small thing in web development, powered by Svelte
https://sapper.svelte.dev
MIT License
7k stars 434 forks source link

Resolve issues of SAPPER_EXPORT variable #1693

Closed the-homeless-god closed 3 years ago

the-homeless-god commented 3 years ago

Some repositories that will use Sapper in development will face with issue: 'process.send not found'

It happens when user provides own variable SAPPER_EXPORT in environment, but user not provided 'process' in package. As I understood, the main goal in checking for environment variable. Currently, get_server_route_handler just checks the existing of variable instead a value

More details in issue of repository that uses Sapper: https://github.com/Zimtir/SENT-template/issues/114

Another solution: write in README.md that user should be sure when provides a value into SAPPER_EXPORT variable

Before submitting the PR, please make sure you do the following

Tests

the-homeless-god commented 3 years ago

@benmccann PTAL when you will have a time

benmccann commented 3 years ago

I don't understand your explanation of what is wrong

the-homeless-god commented 3 years ago

@benmccann process.send is not a function error appears when user defined SAPPER_EXPORT in own environment, but not provided dependency to process. Simple fix is checking of value for SAPPER_EXPORT variable instead the check of the existing for variable in environment. PTAL in code changes of PR

benmccann commented 3 years ago

What do you mean by "not provided dependency to process"?

the-homeless-god commented 3 years ago

@benmccann I mean situation when process.send not defined in environment, for example browser environment

This issue already resolved in some parts of Sapper, but repository not contains the resolving for place in get_server_route_handler

A search through repository on the screenshot below, as you can see in two places process.send checked to exist before the calling

image

And checking of variable will check only the existing of variable, look at screenshot below

image

Because:

const variable = process.env.SAPPER_EXPORT

// will be true when any value (even 'true' and 'false') pasted in SAPPER_EXPORT
// will be false for undefined and null
if (variable) {}
benmccann commented 3 years ago

I'm still not following. I've never used sapper export and don't know this code that well so need more explanation of what's happening. Are you saying that someone is passing "false" into SAPPER_EXPORT? If so, why or how is it getting there?

Conduitry commented 3 years ago

No one should be manually passing any values into an environment variable called SAPPER_EXPORT - that's intended to be an internal Sapper thing. I don't think this is a situation that we want to cater to.

JSON.parse(process.env.SAPPER_EXPORT || null) isn't a good thing to be doing anyway, as that will throw an exception if the environment variable isn't parseable as JSON.

The only way I can see this being an issue that this PR would fix is if someone is running Sapper with SAPPER_EXPORT manually set to a non-empty string that encodes a falsy value - and that's not a supported use case. This isn't something you should be doing.