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

New process definition in html-manager/webpack.config.js conflicts with getOption helper of @jupyterlab/coreultils #3306

Closed
trungleduc opened this issue Nov 3, 2021 · 8 comments · Fixed by #3311
Assignees

Comments

@trungleduc
Copy link
Contributor

Description

The process variable defined in html-manager webpack configuration

new webpack.DefinePlugin({
// Needed for Blueprint. See https://github.com/palantir/blueprint/issues/4393
'process.env': '{}',
// Needed for various packages using cwd(), like the path polyfill
process: { cwd: () => '/' },
}),

is also used in getOption helper of @jupyterlab/coreultils

try {
  const cli = minimist(process.argv.slice(2));
  const path: any = require('path');
  let fullPath = '';
  .....
} catch (e) {
  console.error(e);
}

since this variable will be replaced with value defined in webpack configuration at compile time, this leads to the output of the html-manager bundle is:

try {
    const cli = minimist_1.default({
        cwd: ()=>"/"
    }.argv.slice(2))
        , path = __webpack_require__(21023);
    let fullPath = "";
    ...
} catch (e) {
    console.error(e)
}

which will obviously raise an error since {cwd: ()=>"/"}.argv is undefined

Reproduce

Build html-manager and search for const cli in dist/embed.js

Expected behavior

The correct output should be:

try {
    const cli = minimist_1.default(process.argv.slice(2))
      , path = __webpack_require__(51);
    let fullPath = "";
    ...
} catch (e) {
    console.error(e)
}

Context

  • ipywidgets version: master
@trungleduc
Copy link
Contributor Author

cc @martinRenou

@jasongrout
Copy link
Member

is also used in getOption helper of @jupyterlab/coreultils

try {
  const cli = minimist(process.argv.slice(2));
  const path: any = require('path');
  let fullPath = '';
  .....
} catch (e) {
  console.error(e);
}

Just above these lines, there is the conditional:

https://github.com/jupyterlab/jupyterlab/blob/070227a403f7073047c201c77599ac861376b793/packages/coreutils/src/pageconfig.ts#L55

    if (!found && typeof process !== 'undefined' && process.argv) {

Since process.argv is undefined, how is this invoking the code you mention above?

@trungleduc
Copy link
Contributor Author

trungleduc commented Nov 4, 2021

Hi @jasongrout, here is the output of webpack of this block code

if (!found)
    try {
        const cli = minimist_1.default({
            cwd: ()=>"/"
        }.argv.slice(2))
          , path = __webpack_require__(21023);
        let fullPath = "";
        "jupyter-config-data"in cli ? fullPath = path.resolve(cli["jupyter-config-data"]) : "JUPYTER_CONFIG_DATA"in {} && (fullPath = path.resolve({}.JUPYTER_CONFIG_DATA)),
        fullPath && (configData = eval("require")(fullPath))
    } catch (e) {
        console.error(e)
    }

And here is the output if I remove the process variable definition.

if (!found && "undefined" != typeof process)
    try {
        const cli = minimist_1.default(process.argv.slice(2))
          , path = __webpack_require__(21023);
        let fullPath = "";
        "jupyter-config-data"in cli ? fullPath = path.resolve(cli["jupyter-config-data"]) : "JUPYTER_CONFIG_DATA"in {} && (fullPath = path.resolve({}.JUPYTER_CONFIG_DATA)),
        fullPath && (configData = eval("require")(fullPath))
    } catch (e) {
        console.error(e)
    }

Maybe webpack did some magic things with the process variable?

@jasongrout
Copy link
Member

Maybe webpack did some magic things with the process variable?

Oh wow, that is very interesting. Yeah, why is webpack not transliterating that code and instead removing conditions?

@martinRenou
Copy link
Member

@trungleduc how can we reproduce this error?

@trungleduc
Copy link
Contributor Author

Sorry for my late reply, you can use nbconvert to convert a notebook into HTML with --HTMLExporter.widget_renderer_url pointing to .../ipywidgets/packages/html-manager/dist/embed-amd.js

@martinRenou martinRenou self-assigned this Nov 16, 2021
@martinRenou
Copy link
Member

Assigning myself

@martinRenou
Copy link
Member

martinRenou commented Nov 16, 2021

Using the following polyfill:

new webpack.DefinePlugin({
  // Needed for Blueprint. See https://github.com/palantir/blueprint/issues/4393
  'process.env': '{}',
  // Needed for various packages using cwd(), like the path polyfill
  'process.cwd': '() => "/"',
  // Needed for various packages using argv(), like JupyterLab's getOption function
  'process.argv': 'undefined'
}),

It does not fail anymore, but the resulting if condition is still pretty weird.

if (!found && "undefined" != typeof process)
  try {
      const cli = minimist_1.default((void 0).slice(2))
        , path = __webpack_require__(21023);
      let fullPath = "";
      "jupyter-config-data"in cli ? fullPath = path.resolve(cli["jupyter-config-data"]) : "JUPYTER_CONFIG_DATA"in {} && (fullPath = path.resolve({}.JUPYTER_CONFIG_DATA)),
      fullPath && (configData = eval("require")(fullPath))
  } catch (e) {
      console.error(e)
  }

Though it doesn't go through the condition because process is not defined anymore.

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 a pull request may close this issue.

3 participants