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

Can't find node's modules ... client side. #3488

Closed
ghost opened this issue Jun 12, 2015 · 10 comments · Fixed by #4308
Closed

Can't find node's modules ... client side. #3488

ghost opened this issue Jun 12, 2015 · 10 comments · Fixed by #4308
Labels
API Relates to the public API for TypeScript Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@ghost
Copy link

ghost commented Jun 12, 2015

I am using typescript client side in a project managed using web pack.

When web pack processes typescriptServices.js, some part of the getNodeSystem code is evaluated because (typeof module !== "undefined" && module.exports) is true.

Evaluating the code, web pack finds some references to require calls and try to find modules, it fails to do so because I don't use node modules client side.

Is there a way for you to get rid of this problem ? Perhaps it is possible to split the library depending on if it is used client or server side ?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 12, 2015

well.. all what this does is it says if your system supports importing and exporting, it will export a value. I am not familiar with webpack can you explain more what happens when the export is invoked?

@mhegazy mhegazy added the API Relates to the public API for TypeScript label Jun 12, 2015
@ghost
Copy link
Author

ghost commented Jun 16, 2015

What it does is module and module.exports are defined, then the function getNodeSystem is executed and it search for node modules.

My problem is that I am in a browser, I have defined module and module.exports but as I am client side I have no node modules installed, then getNodeSystem execution failed.

If my module and module.exports properties were named different, I probably wouldn't have any problem, but I can't name them differently.

Did I give you enough information ?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 16, 2015

so what would be a better condition to check for node but not run into webpack? currently it is:

typeof module !== "undefined" && module.exports

@mhegazy mhegazy added the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Jun 16, 2015
@vladima
Copy link
Contributor

vladima commented Jun 17, 2015

what about checking global variable process and probably some of its properties

typeof process == "object" && process.versions && process.versions.node && process.versions.v8
Object.prototype.toString.call(global.process) === '[object process]'

@mhegazy
Copy link
Contributor

mhegazy commented Jun 18, 2015

Pinging @csnover for this one. @csnover any insight on the correct check here that would work with node-like loaders e.g. browserfiy and webpack?

@csnover
Copy link
Contributor

csnover commented Jun 18, 2015

There isn’t any way to detect Node.js as a distinct environment any more because it isn’t. Electron and NW.js for example stick Node.js APIs into a browser context, where you have the ability to do script injection to load any code like a browser, but also you can call sync require to load local code like Node.js.

So, all you can try to do as an author is attempt to infer that the APIs you actually are going to call exist, and then pray that they’re actually compatible. So if you are doing something like require('fs') then some code like this:

function getFs() {
  if (typeof require === 'function') {
    try {
      var fs = require('fs');
      if (looksLikeFs(fs)) {
        return fs;
      }
    }
    catch (error) {
      return null;
    }
  }
}

I’ll leave it up to you to try to figure out how to do it in a browser.

In the case of putting typescriptServices.js into webpack, it sounds like that’s simply not supported and if users want to bundle it with something else for use in the browser they need to use a straight concatenative approach instead of one that wraps the code with Node.js-ish APIs that confuse/defeat the environment detection.

@ghost
Copy link
Author

ghost commented Jun 19, 2015

@mhegazy @vladima @csnover thank you guys for the time you spent analyzing my problem and suggesting solutions.

I found a hack fitting my architecture in two steps :
First, I exclude typescriptservices.js from web pack processing, thus, I avoid the failure of generating the production environment.
Finally, I load the content of typescriptservices.js, create a new function which body is the content of the file and return the symbol I need.

It works, and avoid me having to patch typescriptservices.js :)

@mhegazy mhegazy added the Bug A bug in TypeScript label Jun 19, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Aug 12, 2015

pinging @jbrantly.

@jbrantly any ideas here on loading issue with webpack?

@jbrantly
Copy link

This has been a fun one. I tried skirting around the issue in webpack using the official imports-loader and exports-loader but couldn't find a way to make it work. I was able to make it work using an unofficial loader called wrap-loader.

// webpack.config.js
module: {
    noParse: [/typescript.js$/],
    loaders: [
        { test: require.resolve('typescript'), loader: "wrap?typescript" }
    ]
},
wrap: {
    typescript: {
        before: [
            'var __module = module;',
            'module = false;'
        ],
        after: ['__module.exports = ts;']
    }
}

// app.js
var typescript = require('typescript');

console.log(typescript.transpileModule('let foo = 0;', {compilerOptions: {}}).outputText)

This works by first making sure webpack does not parse TypeScript for require by using the noParse config. Secondly it wraps TypeScript, renaming module so that the current node behavior doesn't get picked up but still allowing for an export at the end using the renamed variable.

Looking at this, I would say that it's weird that TypeScript uses the current typeof module !== "undefined" && module.exports check for node. Really that check is for CommonJS compatibility, not node compatibility. I would suggest something like

typeof process !== "undefined" && !process.browser

That approach, in my mind, leans toward the current behavior while enabling compatibility with webpack and browserify. In node and most node-like environments, the node path will be taken, unless the node-like environment explicitly states that its in the browser (both webpack and browserify set process.browser).

@jbrantly
Copy link

One refinement: typeof process !== "undefined" && !process.browser is probably too liberal and can pick up a stray process variable. Probably also need a positive property check as @vladima proposed. LESS uses typeof process !== "undefined" && process.nextTick for this purpose. Something like typeof process !== "undefined" && process.nextTick && !process.browser seems reasonable to me. The first two conditions include all node-like environments and the last excludes webpack and browserify.

@mhegazy mhegazy added this to the TypeScript 1.6 milestone Aug 13, 2015
@mhegazy mhegazy removed the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Aug 13, 2015
@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Aug 13, 2015
@vladima vladima added the Fixed A PR has been merged for this issue label Aug 14, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Relates to the public API for TypeScript Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants