first baby steps towards native browser support for the JS sdk of testground by GlenDC · Pull Request #25 · testground/sdk-js

GlenDC · 2022-10-27T17:28:20Z

I am by far no NodeJS expert, so please do help me adapt to this culture where you see fit.

With this PR I distinguish in some first starter locations between the browser and node environment. Currently I do this using process.title:

within the NodeJS environment this is the value of your ps (see: https://nodejs.org/api/process.html#processtitle)
when web-packing sdk-js with the process capabilities poly-filled (shimmed?) using the process shim package it sets this to browser as the title: https://github.com/defunctzombie/node-process/blob/master/browser.js#L155

Seems to me like a clean way, but if there's a more natural way, do let me know.

Currently there are the following changes where we differentiate between browser and NodeJS:

creating a runtime uses the process.env, this is however not available in browser (and it is empty using the shim pacakge), so here we opt to assume that the env variables are available under window.testground.env, which are to be injected by the user of the js sdk in the browser;
for the logging we currently also create a file on the local FS (the output log file?), this is not possible in browser, so I just disable that single output, other outputs do still function;

This is just the beginning though, I imagine there is even more we can do and want to do if native browser support for sdk-js is what we want. My current aim is to make it as easy to use as in NodeJS, meaning that the code can just be used and it will pull all it needs in the background (e.g. env variables).

Things to keep in mind...

for now we still need to polyfill a lot when web-packing, e.g. this is what I have for a testground plan in my webpack config file:

resolve: {
    extensions: ['.js'],
    fallback: {
      fs: false,
      os: require.resolve('os-browserify'),
      path: require.resolve('path-browserify'),
      https: require.resolve('https-browserify'),
      crypto: require.resolve('crypto-browserify'),
      http: require.resolve('stream-http'),
      zlib: require.resolve('browserify-zlib'),
      buffer: require.resolve('buffer/'),
      url: require.resolve('url/'),
      'assert/': require.resolve('assert/'),
      stream: require.resolve('stream-browserify')
    }
  },
  plugins: [
    new webpack.ProvidePlugin({
      Buffer: ['buffer', 'Buffer']
    }),
    new webpack.ProvidePlugin({
      process: 'process/browser'
    })
  ],

Could be great if out of the box, the sdk-js already does a lot of that for us, or ensures that what it uses is more cross-platform and thus does not need to be shimmed?

I am no expert of this library, so there might be even more places where we need to add some different logic for clean browser support. Please do point me to these locations if you know of any.

Finally we might also want to add some documentation on pointers for some gotcha's, hints, etc. All related to usage in browser.

- switch to polyfill process name for browser detection as to make this automatic, rather than explicit - make currentRunEnv also work from a browser

GlenDC · 2022-10-27T17:30:24Z

src/runtime/logger.js

    transports.push(new winston.transports.File({
      format,
-      filename: path.join(params.testOutputsPath, 'run.out')
+      filename: 'stdout'


I believe this is due to a merge from an older master version. Should I remove this all together?

I don't understand, what did you merge? What would you like to remove?

not sure anymore, going to clean it up.

laurentsenta · 2022-11-01T15:32:24Z

src/runtime/index.js

+    return parseRunEnv(window.testground.env)
+  } else {
+    return parseRunEnv(process.env)
+  }


~~Is this standard? Else, could we simplify with something like process.env || window.testground.env?~~

Got it, that's in the PR description.
Could we move the checks like if proces.title into a single function if (isBrowser()) for example?

Sure thing.

Will drop this in favour of the browser spec.

laurentsenta · 2022-11-01T16:06:48Z

src/runtime/logger.js

+    }
  }

  return winston.createLogger({


(creating a thread)

You listed a lot of fallback libraries, do you know what purpose they solve? (for example the path library might "just" be required by the winston logger).

@achingbrain shared an interesting option: It might be enough to use the browser field to shim the features that are not isomorphic (the loggers creation and the env retrieval): https://github.com/defunctzombie/package-browser-field-spec#replace-specific-files---advanced

They solve the purpose of being able to run the testground plan without runtime errors, so that collection get assembled in a trial-error approach. I'll look into the browser field, haven't used that before, so if you could give some practical example of how you see that that would be great. Either way I'll give it a look.

Ok got'cha, you shared it in the other thread already :)
(#25 (comment))

If we want to get rid of the fallback libraries however we might need to either make winston-js browser friendly (e.g. see winstonjs/winston#2196) or from our end (sdk-js) make the logging different for browser vs node.

This winstonjs/winston#287 was opened in 2013

It's very likely this will take us month(s) to get browser support in winston,
I'd recommend you try this: https://stackoverflow.com/a/55433049/843194
and else, "just" return a console object when we're in the browser.

We can tackle full support for logging as a follow-up issue (related: testground/testground#1355)

Not to worry, that’s already an approach I’m taking on my local replace branch. Sorry if i didn’t communicate that yet

laurentsenta · 2022-11-01T16:09:18Z

src/runtime/logger.js

+    }
  }

  return winston.createLogger({


(creating a thread)

You listed a lot of fallback libraries, do you know what purpose they solve? (for example the path library might "just" be required by the winston logger).

@achingbrain shared an interesting option: It might be enough to use the browser field to shim the features that are not isomorphic (the loggers creation and the env retrieval): https://github.com/defunctzombie/package-browser-field-spec#replace-specific-files---advanced

A few examples he shared:

// get-env.js export function getEnv() { return process.env } // get-env.browser.js export function getEnv { return window.testground.env } // package.json "exports": { // used by external imports "./get-env": { "browser": "get-env.browser.js", "import": "get-env.js" } }, "browser": { // used by internal imports (e.g by esbuild before building code for running unit tests) "get-env.js": "get-env.browser.js" }

Oh ok nice, so kinda like what you would do with C macro's in C/C++, go file suffixes and cfg-like macro's in Rust. Got-cha. That does look a lot cleaner indeed. Will play with it this week and see how far I get.

Ok read the spec, seems fine. Going to adapt the PR to this format.

GlenDC · 2022-11-09T23:48:31Z

Replaced this PR with #26.

- use shims + browser field to support - add registerTestcaseResult to communicate end of test with a node runner when running in-browser - Expose getEnvParameters to let a client gather and move testground variables (used for browser support). - Supersede #25, having applies all the previous feedback.

glendc added 5 commits October 1, 2022 09:13

expose parseRunParams of runtime

da26edf

do not log using File (doesn't work in browser)

40263a8

do not expose parseRunParams any longer

44545a8

improve browser support

3267223

- switch to polyfill process name for browser detection as to make this automatic, rather than explicit - make currentRunEnv also work from a browser

s/process.name/process.title/g

2191269

GlenDC commented Oct 27, 2022

View reviewed changes

This was referenced Oct 27, 2022

examples: add node & cross-browser example testground/testground#1502

Merged

EPIC: implement Javascript & Browser support in Testground testground/testground#1386

Closed

laurentsenta reviewed Nov 1, 2022

View reviewed changes

GlenDC mentioned this pull request Nov 9, 2022

feat: add basic browser support #26

Merged

GlenDC closed this Nov 9, 2022

Comments

Conversation

GlenDC commented Oct 27, 2022 • edited by laurentsenta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurentsenta Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GlenDC commented Nov 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

GlenDC commented Oct 27, 2022 •

edited by laurentsenta

Loading

laurentsenta Nov 1, 2022 •

edited

Loading