electron-userland / electron-compilers

DEPRECATED: Compiler implementations for electron-compile
35 stars 55 forks source link

TypeScript compiler regression #52

Closed malept closed 7 years ago

malept commented 7 years ago

The commit https://github.com/electron/electron-compilers/commit/ffbd7002b66c0ce9c4d801bdbce7569997b7ad1c introduced a regression. The following repro steps fail with 5.3.0 and 5.3.1 and succeeds with 5.2.5.

Repro steps

  1. Install Electron Forge
  2. Run electron-forge init something
  3. Run cd something
  4. Run electron-forge package

Expected

A package is created for the host platform & arch.

Actual

✔ Checking your system
✔ Preparing to Package Application for arch: x64
⠋ Compiling ApplicationCouldn't set up compilers: Cannot find module 'typescript/package.json'
✖ Compiling Application

An unhandled error has occurred inside Forge:
Cannot find module 'typescript/package.json'
Error: Cannot find module 'typescript/package.json'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at TypeScriptCompiler.getCompilerVersion (/tmp/something/node_modules/electron-compilers/lib/js/typescript.js:70:12)
    at Object.keys.reduce (/tmp/something/node_modules/electron-compile/lib/compiler-host.js:496:35)
    at Array.reduce (native)
    at CompilerHost.saveConfigurationSync (/tmp/something/node_modules/electron-compile/lib/compiler-host.js:488:72)
    at createCompilerHostFromConfiguration (/tmp/something/node_modules/electron-compile/lib/config-parser.js:323:7)
    at /tmp/something/node_modules/electron-compile/lib/config-parser.js:80:12
    at next (native)
    at step (/tmp/something/node_modules/electron-compile/lib/config-parser.js:179:191)
    at /tmp/something/node_modules/electron-compile/lib/config-parser.js:179:361

When npm install is run, this is one of the warnings (electron-compilers@5.3.1 is installed):

UNMET PEER DEPENDENCY typescript@>=1.6
MarshallOfSound commented 7 years ago

/cc @kwonoj @paulcbetts typescript shouldn't be a required peer-dep as lots of people use electron-compile without using typescript.

/cc @malept Can we pin our electron-compile version until this is fixed

kwonoj commented 7 years ago

@MarshallOfSound so previously, typescript was dependency regardless of someone uses TS or not (https://github.com/teppeis/typescript-simple/blob/master/package.json#L28), this regression comes by now compiler correctly requires compiler optional, just missing correct handling if compiler is really missing.

Compiler behavior should be fixed to work without installation of TS but I still think typescript installation need to be optional instead of requiring it in behind of scene. maybe optionalDep would be more sense than peerDep though.

MarshallOfSound commented 7 years ago

@kwonoj Fair enough, however I think that the change that was made should have been considered breaking. Anyone currently using typescript will find themselves unable to do so with just a minor bump in this package. Is that correct?

kwonoj commented 7 years ago

@MarshallOfSound for versioning perspective, I do agree. actually, when I proposed PR (https://github.com/electron/electron-compilers/pull/50) I proposed it with breaking changes.

kwonoj commented 7 years ago

@MarshallOfSound please review https://github.com/electron/electron-compilers/pull/53, I've added handler also made TS compiler as direct dependency to avoid regression in minor version bump.

I'll create separate PR for making TS compiler as peerDep for major version bump.

anaisbetts commented 7 years ago

Sorry all, I didn't remember that we'd check the version, totally agree that this should be fixed asap

malept commented 7 years ago

Fixed, thanks Paul.