Skip to content
This repository has been archived by the owner on Apr 30, 2019. It is now read-only.

Regression in 0.6.3 #179

Closed
jimlloyd opened this issue Jun 23, 2015 · 14 comments
Closed

Regression in 0.6.3 #179

jimlloyd opened this issue Jun 23, 2015 · 14 comments
Assignees

Comments

@jimlloyd
Copy link

A clean build of our large mult-module package now fails due to a change introduced in 0.6.3. We had been using 0.6.1 successfully. Although npm info tsd claims a version 0.6.2 was released, there is no tag for that version, and npm fails if an attempt is made to install [email protected].

The failure we are seeing with [email protected] is that the typings directory remains empty despite console output indicating that files were created. This failure mode happens for me on my Mac development machine and also on our Jenkins CI server (linux).

@Diullei
Copy link
Member

Diullei commented Jun 24, 2015

Hi @jimlloyd

The 0.6.2 version was released with an inconsistent build, so we removed this version from npm.

I'm trying replicate your issue, can you provide the tsd command line that is failing and the content of your tsd.json?

@jimlloyd
Copy link
Author

Hi @Diullei,

Ok I have dug deeper and can give you more specific information, but I may need my colleague @mhfrantz to chip in, and he is unfortunately on vacation this week.

In each of our modules we have a Makefile that does these tsd-related steps:

npm install
tsd reinstall
tsd --config node_modules/tsd.json reinstall

In the first step, npm install may install some of our packages which have as a postinstall step a call to ts-pkg-installer. After npm install is complete, our typings directory will have one directory named @redseal, with one or more directories containing .d.ts files declaring the interfaces of our modules, and the node_modules directory may have a tsd.json file.

The tsd reinstall step should add to the typings directory the .d.ts files for third party modules. As of 0.6.3, it seems to blow away the @redseal directory. Does reinstall now mean 'clean reinstall'?

The final step running tsd reinstall with the config file node_modules/tsd.json is supposed to update the typings directory with interfaces hoisted up from dependencies installed in node_modules. However, it too seems to blow away the contents of typings. All of our Makefiles do all three of the tsd-related steps above. For some 'leaf' modules in our dependency tree, the node_modules/tsd.json file is not created, yet we still run the 3rd step above. That step now silently empties our typings directory.

So, I believe all of the difficulty we are having with 0.6.3 can be attributed to the fact that tsd reinstall seems to now clean the typings directory first before doing the reinstall. It did not do that in 0.6.1.

I haven't looked at the changes made between 0.6.1 and 0.6.3. I don't see anything in the README to explain this behavior. Please let us know if this new behavior is intentional, and what we should do to get the previous behavior.

@Diullei
Copy link
Member

Diullei commented Jun 24, 2015

Ok, now I got your issue. This is happening because a new implementation that clean the typings when the uninstall command is performed. (Related here: #151 #167). On your specific case you use more than one tsd.json. I not figured this scenário, sorry for this break change.

Let's propose a good approach to solve this issue. For now keep using the 0.6.1 version.

I think the reinstall command make more sense cleaning the not referenced typings, but we can provide a flag to force tsd to keep this typings. Something like:

tsd reinstall --keep-unreferenced

What do you think?

Sorry again, for me it is very important to resolve it. I'll do my best to help you with this issue.

@jimlloyd
Copy link
Author

The --keep-unreferenced flag sounds reasonable. The other option would be to add a new flag --clean, and make reinstall only invoke the 0.6.3 behavior when that flag is present.

But I can see that 'reinstall' almost seems to imply 'clean'. If we are the only user who was broken by this change then --keep-unreferenced may be the best solution. We can work with whatever you decide to do, now that you know our use case.

Thanks,
Jim

@Diullei
Copy link
Member

Diullei commented Jun 24, 2015

Thanks Jim!

I don't know if another users has been affected by this change, but I'm monitoring the issues.

For now you need keep using the 0.6.1 version. I'll try work with the new approach ASAP.

@jimlloyd
Copy link
Author

Yeah, we're pinned to semver =0.6.1 for now, which is fine. Update this issue when you're ready for us to try out your fix.

@Diullei
Copy link
Member

Diullei commented Jun 28, 2015

Hi @jimlloyd

I implemented the --keep-unreferenced flag on the latest merge #180. Could you verify if this solve your issue?

Steps to install the latest tsd version from GitHub:

Clone the tsd repo and build the project

git clone https://github.com/DefinitelyTyped/tsd.git

go to tsd directory and run:

npm install -g grunt-cli
npm install

# to build
grunt

put this tsd build a global npm module. From inside the tsd directory run:

npm link

execute your build process...

NOTE: add the --keep-unreferenced flag to your reinstall command

My test case...

I'm using the following test case to reproduces your scenario:

Having a directory with 2 files: tsd.json and tsd-2.json where tsd.json reference the jquery definition and tsd-2.json reference qunit definition I execute the following commands:

tsd reinstall
tsd reinstall --keep-underefenced --config tsd-2.json

So, I'll have a typings directory with jquery and qunit definitions installed.

https://github.com/DefinitelyTyped/tsd/tree/master/test/nspec/cases/reinstall-keep-unreferencced-two-tsd-json

I got success with this test.

https://travis-ci.org/DefinitelyTyped/tsd/jobs/68688565#L1236
https://ci.appveyor.com/project/Diullei/tsd/build/3#L1142

NOTE:
Now, if you got an error, tsd will generate a tsd-debug-log file with the error details. Please, provide this file if you got some problem.

@jimlloyd
Copy link
Author

Hi @Diullei, thanks for the update. I'm on vacation this week but do have my laptop. I may not have time to work on this today but I should be able to get to it in the next few days.

@Diullei
Copy link
Member

Diullei commented Jun 29, 2015

No problem 👍

@jimlloyd
Copy link
Author

Hi @Diullei, my apologies for not jumping on this right after getting back from vacation. I'm trying to do it now and running into a problem with the build. When running npm install I see these strange errors:

Running "ts:api" (ts) task
Compiling...
Using tsc v1.5.3
/Volumes/Sneaker/github/bluebird.d.ts(711,2): error TS2300: Duplicate identifier 'export='.
/Volumes/Sneaker/github/tsd/typings/bluebird/bluebird.d.ts(669,2): error TS2300: Duplicate identifier 'export='.

>> 2 non-emit-preventing type warnings  
>> Error: tsc return code: 2
Warning: Task "ts:api" failed. Use --force to continue.

Aborted due to warnings.

I immediately suspected the problem was some incompatibility with the recently released [email protected], but I get the same error if I force the installation of [email protected].

Those errors are also disturbing because of the first path: /Volumes/Sneaker/github/bluebird.d.ts. My tsd is installed in /Volumes/Sneaker/github/tsd so the build was looking for a bluebird.d.ts file outside of the project directory.

@jimlloyd
Copy link
Author

jimlloyd commented Aug 3, 2015

@Diullei I dug deeper and discovered the above build error was triggered by the fact that I did have a copy of bluebird.d.ts in the parent directory. It's disturbing that the existence of the file outside of the project directory caused typescript to fail to compile, but I was able to work around the bug by removing the file. This let me create a clean build of tsd, and then test the --keep-unreferenced flag. It seemed to work as expected.

I see from discussion in #184 that you're still thinking about --keep-unreferenced vs. --clean vs. other options. I marginally prefer --clean, but am open to other options.

mhfrantz pushed a commit to RedSeal-co/ts-pkg-installer that referenced this issue Aug 14, 2015
The only exception is tsd, which must stay at 0.6.1 until this issue in 0.6.3 is resolved:

DefinitelyTyped/tsd#179
@Diullei
Copy link
Member

Diullei commented Aug 29, 2015

@jimlloyd I published a new version to npm (0.6.4) with the fix (using --clean flag). Thank you!

@Diullei Diullei closed this as completed Aug 29, 2015
@jimlloyd
Copy link
Author

Hi @Diullei, I just wanted to close the loop here and let you know that 0.6.4 does fix our problem. Thanks!

@Diullei
Copy link
Member

Diullei commented Aug 31, 2015

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants