Changed setting of `isNodeLike` boolean

[cliener]

…to sniff for the specific process features required since browser sometimes incorrectly return true.

Potential fix for #71

[CLAassistant]

All committers have signed the CLA.

[EisenbergEffect]

@cliener For my own learning, in what scenarios does that return true in the browser?

[cliener]

By all rights, the old code shouldn't return true but does anyway in circumstances I've been unable to isolate. The change sniffs the particular properties used in line 83 when isNodeLike is true thus ensuring a false positive won't break.

[cliener]

I realise I've neglected to build the code in my fork. I'll get that sorted as soon as I fix an unrelated Gulp/Node error.

[cliener]

Good to go!

[EisenbergEffect]

@cliener I think I misunderstood your last comment. I'm not sure what I was thinking actually. So, I apologize. We actually don't commit the dist folder except when I push an official build out. Can you remove the dist folder changes from this PR please? Again, I apologize.

[cliener]

No worries. Rolled back.

[EisenbergEffect]

Ok, before we get this in, I'd like to get @timfish to make a review, if possible. He was someone who contributed to this area of the code with some specific scenarios around NW.js. I want to make sure we don't accidentally break NW.js integration with this. @timfish Can you take a look?

[timfish]

This PR removes the check for !process.browser which I think is/was Webpack/Browserify specific. Would removing this be a semver major change?

In the browser, the only way the current typeof process !== 'undefined' && !process.browser will return true is if you've defined a global process somewhere. @cliener, have you checked your code to see if there are any variables called process that don't have a var/let/const in front of them and have leaked into the global scope?

The change you have proposed will result in isNodeLike = false on Electron because typeof process.type === 'string'.

How can we improve the existing code? The isNodeLike detection is used for two things.

1 - Work out whether to use aurelia-loader-nodejs

    if (isNodeLike && typeof module !== 'undefined' && typeof module.require !== 'undefined') {
      // note: we use a scoped module.require() instead of simply require()
      // so that Webpack's parser does not automatically include this loader as a dependency,
      const m = module.require('aurelia-loader-nodejs');
      return Promise.resolve(new m.NodeJsLoader());
    }

In this case, the user has not configured any other loader and this is the last attempt before throwing, so it's probably enough to simply check:

    if (typeof module !== 'undefined' && typeof module.require !== 'undefined') {

If module and module.require are available we know we're in something that supports these so shouldn't that be enough?

2 - Work out which PAL to load and force browser for Electron and NW.js 'front-ends'

  const isRenderer = isNodeLike && (process.type === 'renderer' || process.versions['node-webkit']);

  if (isNodeLike && !isRenderer) {
    type = 'nodejs';
  } else if (typeof window !== 'undefined') {
    type = 'browser';
  }

The issue with the existing code above is that accesses process.versions['node-webkit'] without first checking that versions exists.

All we really want to know is whether this is 'browser-like' so it's probably a better idea to check for the existence of window first by reordering the if statements:

  // This is true in the browser or any other env that is 'browser-like', ie. Electron and NW.js 
  if (typeof window !== 'undefined') { 
    type = 'browser';
  } else if (typeof process !== 'undefined') {
    // We'll only end up here in nodeOnly processes
    type = 'nodejs';
  } else 

@EisenbergEffect What do you think? Would the above window detection break anything with SSR or node unit tests?

[cliener]

@timfish I absolutely agree in principle and checking for shadowed variables was the first thing I did. If you check the screen shot in #71, you'll see process returns inconsistently between the console and inspector.

[timfish]

I saw the screenshot but I'd be more interested in seeing the value of process because it must have a value for isNodeLike to be true.

The default empty Webpack configuration mocks node process, so the code definitely needs improving. If you're using Webpack do you have node: false in your config?

Almost every single consumer of Aurelia will hit these code paths, so before we modify them, we need to fully understand what's causing the issue we're trying to fix.

[EisenbergEffect]

@cliener It would be handy to see what the value of process was. Seems possible that it's a Webkit mock thing going on. We just need to be careful of changes here.

[cliener]

Ahh sure. In the console, process returns undefined. In the code inspector, it shows object. Every attempt I've had at finding out more than object has returned undefined

[EisenbergEffect]

Sorry to take so long getting back to this. Lots of stuff going on the last few weeks.

So, it looks like process is probably getting mocked by Webpack, complicating the various checks. Any recommendations on what we should do? Here are a few thoughts:

• I'd like to avoid a major version bump (breaking change) if we can.
• We need to make sure things still work properly in Electron and NWJS, in addition to the browser.
• We need to make sure all our bundlers/loaders continue to work correctly: Webpack, Require.js, System.js.

The good news is that this is all addressed for vNext, due to some different architectures allowing us to eliminate these types of checks. But, I'd like to address this for vCurrent. What's the smallest change we could make?

[cliener]

Hence my theory of testing only for what's required. Does this change potentially harm anything?

[EisenbergEffect]

Let me just get a couple more eyes from the core team on this to make sure before we push it in. Some relevant folks includ @3cp , @fkleuver , and @bigopon who have various experience with the varieties of contexts that we need this to function in. Can you guys each take a quick look?

[3cp]

When webpack/browserify/parcel/cli-bundle stubs process, they all use the same npm package "process".

That "process" package is designed to be used in browser, it has a flag "browser" set to true. That's why the existing code checks that flag. This is not webpack/browserify specific, it's almost all bundlers.

https://github.com/defunctzombie/node-process/blob/77caa43cdaee4ea710aa14d11cea1705293c0ef3/browser.js#L156

When webpack/browserify stubs process, it's a local variable but looks like a global var in code, you cannot get it through windows.process variable, that's why you see undefined.

Your original issue on 'node-webkit' might be a different thing. You can easily debug process in your code everywhere by just print it as "global" var. For example:

app.js

console.log('process.browser', process.browser);
console.log('process', process);

export class App {...}

[fkleuver]

I would definitely do the isBrowserLike test first and if it's true, assume it's a browser. AFAIK only jest sets the window global in node and when it does, by all means and purposes code should be able to work as if it were a real browser anyway, that's the point of jest. (but don't use jest please, it's slow, bad and flaky)

Actually in vNext we're not even setting any kind of type property anymore, what I've done is simply expose 3 variables: https://github.com/aurelia/aurelia/blob/master/packages/kernel/src/platform.ts#L48 PLATFORM.isBrowserLike PLATFORM.isWebWorkerLike PLATFORM.isNodeLike

This is purely for end users. vNext's PLATFORM initialization actually doesn't rely on any of this. Every single platform-dependent feature is individually checked and appropriately stubbed/polyfilled based on whether it's needed.

vCurrent, on the other hand, decides the complete PLATFORM initialization based on this value. So you're going to get problems if you tell Aurelia that the environment is nodejs when it is in fact the browser. That's why the evaluation order is very important here.

[3cp]

I need to point out our default jest setup didn't use its browser simulation. We turned that off with testEnvironment: 'node'. It's our aurelia-pal-nodejs loads jsdom and simulate window.

[fkleuver]

Oh..I thought we did it the other way around :/

Was there any particular technical reason for that? With that default there is nothing we can do to make it work smoothly in all scenarios actually. Or we'd have to look into that process package and find something we know it doesn't define. Maybe wrap a require('fs') in a try/catch or something. ugh

[3cp]

The jest way works too. aurelia/cli#1019

[EisenbergEffect]

Should we merge this PR to change the test though? Any negative side-effects that you can think of?

[3cp]

As I said, I don't know any situation will lead to process.browser to be false when process exists in browser code. I think we need to understand what went wrong on process.browser first.

[berkerdemirer]

We are having the same issue. Is this PR abandoned for good?

[3cp]

At beginning of #71, we asked a playable demo to reproduce the bug. Unfortunately, the author chooses to ignore the request.

We adopted the common sense practice to reproduce and understand a bug first, before applying a fix.

We think it's little bit irresponsible to release a fix to a bug when we don't know the condition to reproduce it. Hence neither merge nor close this PR. We are merely waiting for more information. We appreciate if you could help us to reproduce the bug. Thanks!

[berkerdemirer]

Understandable. The problem is that we are having this issue in our legacy code and I am not even sure how to reproduce this. All I know is that it broke all of a sudden without us changing anything in the code. That led me to investigate this behavior in different browsers but then again all browsers showed the same result. I can't just pinpoint a single issue why this would happen.

[3cp]

@berkerdemirer what's the configuration of your app? webpack version and webpack config file pls. Or other bundler version and config file.

[3cp]

@berkerdemirer the other debug you can do is:

1. goto your app's node_modules/aurelia-bootstrapper/dist/native-modules/
   • note webpack uses this dist folder, other bundler might use dist/commonjs folder.
2. edit aurelia-bootstrapper.js to add console.log(JSON.stringify(process, null, 2)); just before the line of var isNodeLike =.
3. build and run your app.

See what's printed out in your browser.

[berkerdemirer]

That's the console.

"bootstrap": "github:twbs/bootstrap@3.4.1",


we use gulp v4 and it has series of tasks that it's doing.

[berkerdemirer]

For example, one bizarre behavior is that it breaks whenever I enable one browser extension in Chrome, and if I disable it, it works. But that's my case. We have some customers who don't have that extension and it is still broken for them.

[3cp]

I think some js lib (in your app dependency tree) stubbed process with that env as only property. It's not a proper way to stub process in browser env, it could break many other js packages who checks process.browser. With that env key pair, I should be able to dig up something. I will get back to you.

[berkerdemirer]

After removing that extension causing the error, log returns this.

[3cp]

I only found Web-eID WebExtension referencing this env key. https://github.com/web-eid/web-eid-webextension

[berkerdemirer]

Yeah, as I said many of our customers who has this issue don't even have that extension. So, something in their browsers or something else triggering the same issue.