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.

Sign up for GitHub

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

Jump to bottom

3.6 was missing from Travis / AppVeyor build matrices. #1011

Merged
merged 10 commits into from
Sep 20, 2019
Merged

3.6 was missing from Travis / AppVeyor build matrices. #1011

merged 10 commits into from
Sep 20, 2019

Conversation

johnnyreilly
Copy link
Member

@johnnyreilly johnnyreilly commented Sep 19, 2019 •
edited
Loading

This PR adds it in in the form of TypeScript 3.6.3.

This continues the work of #1003

@johnnyreilly johnnyreilly changed the title Update .travis.yml 3.6.2 was missing from Travis / AppVeyor build matrices. Sep 19, 2019
sheetalkamat
sheetalkamat reviewed Sep 19, 2019
View reviewed changes
.travis.yml Outdated
@@ -14,9 +14,10 @@ install:
- yarn lint
- yarn add $TYPESCRIPT
env:
- [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest changing to 3.6.3 instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup will do. Somehow I managed to forget to add 3.6.x to the build matrix when I upgraded the comparison test pack. Whoops. Fixing now. As far as I can assess there's no problems that have been introduced in the meantime, though I'm regenerating some of the project references test pack output again (which makes sense given the changes that have happened)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will push update in 15 minutes or so

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - now let's see how CI gets on

@johnnyreilly johnnyreilly changed the title 3.6.2 was missing from Travis / AppVeyor build matrices. 3.6 was missing from Travis / AppVeyor build matrices. Sep 19, 2019
@johnnyreilly
Copy link
Member Author

johnnyreilly commented Sep 19, 2019 •
edited
Loading

Hmmm.... There might be a problem with some of the project references tests behaving differently between Windows and Linux:

I'm seeing this on Linux: (Travis)

projectReferencesRootDir:

      -[./lib/src/index.ts] A-NUMBER-OF bytes {main} [built]
      +[./lib/src/index.ts] A-NUMBER-OF bytes {main} [built]
      +ERROR in tsconfig.json
      +[tsl] ERROR
      +      TS5033: Could not write file '/.test/projectReferencesRootDir/lib/out/index.d.ts': Cannot read property 'set' of null.

There's a similar issue with projectReferencesOutDir.

But locally (Windows - where this output was generated) this test passes / ts-loader has different behaviour.

I'm made a fix to improve the resilience of the comparison test pack; there were a few meaningless failures happening due to whitespace differences that aren't significant. Let's see how Travis performs with this in place. Because comparison tests are somewhat flaky we do get failures that succeed on the second attempt. Fingers crossed this is one of those.

Confirmed there does seem to be an issue with projectReferencesOutDir and projectReferencesRootDir. From the looks of it behaviour on Windows (what I'm developing on) is different to behaviour on Linux (Travis).

See example Travis failure here: https://travis-ci.org/TypeStrong/ts-loader/jobs/586873598

cc @andrewbranch as I believe he first wrote these tests and may have some insight to share.

I'm going to hold off releasing 6.1.1 until this is resolved. Thanks for all your work so far @sheetalkamat . I feel we're very nearly there. My apologies for dropping the ball on the build matrix stuff; that's my bad. If you can keep helping out I would be immensely grateful 🤗 (likewise @andrewbranch 🥰)

Linking to PRs that should ship with 6.1.1

#1008
#1003

johnnyreilly
johnnyreilly commented Sep 19, 2019
View reviewed changes
test/comparison-tests/create-and-execute-test.js
@@ -402,9 +402,9 @@ function getNormalisedFileContent(file, location) {
.replace(/Module build failed \(from \//gm, 'Module build failed (from ')
.replace(/Module Warning \(from \//gm, 'Module Warning (from ')
// We don't want a difference in the number of kilobytes to fail the build
.replace(/[\d]+([.][\d]*)? KiB/g, 'A-NUMBER-OF KiB')
.replace(/\s+[\d]+([.][\d]*)? KiB\s+/g, ' A-NUMBER-OF KiB ')
Copy link
Member Author

@johnnyreilly johnnyreilly Sep 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're getting some meaningless failures here related to whitespace in webpack output. These regex changes effectively normalize those differences. We want meaningful failures only 😄

@johnnyreilly johnnyreilly mentioned this pull request Sep 19, 2019
Closed
@sheetalkamat
Copy link
Contributor

Will take a look at it shortly after setting Linux machine. I think it would be something to do with my changes since my changes override @andrewbranch ‘s work for project references ..

@johnnyreilly
Copy link
Member Author

Nice - thanks!

@sheetalkamat
Copy link
Contributor

sheetalkamat commented Sep 19, 2019 •
edited
Loading

@johnnyreilly I fixed this but I cant push to your branch so created #1013

remote: Permission to TypeStrong/ts-loader.git denied to sheetalkamat.
fatal: unable to access 'https://github.com/TypeStrong/ts-loader.git/': The requested URL returned error: 403

@johnnyreilly
Copy link
Member Author

johnnyreilly commented Sep 19, 2019 •
edited
Loading

Great work @sheetalkamat! I've just sent you an invite to join TypeStrong. Once you accept that you should be able to push (you'll be set up the same as @andrewbranch is)

Feel free to merge in if you're happy!

@johnnyreilly
Copy link
Member Author

I've seen enough passing builds to be happy - let's merge this!

@johnnyreilly johnnyreilly merged commit e9ef049 into master Sep 20, 2019
@johnnyreilly johnnyreilly deleted the 3.6.2-is-missing-from-build-matrix branch September 20, 2019 06:49
@johnnyreilly
Copy link
Member Author

johnnyreilly commented Sep 20, 2019 •
edited
Loading

Hey @sheetalkamat / @andrewbranch!

I've merged this PR as all looked good. Interestingly, we just started to get these execution test failures with typescript@next:

2 test suite(s) failed.
 - 3.0.1_projectReferences
 - 3.6.0_projectReferencesToBeBuilt

Seems to be related to this:

TypeError: instance.compiler.getOutputPathForBuildInfo is not a function

Has this function been removed in the last 24 hours?

You can see this here: https://travis-ci.org/TypeStrong/ts-loader/jobs/587342045

I don't think this is a blocker for releasing - I'm planning to ship 6.1.1 shortly. But could you advise on this please?

Thanks!

Update: I'm wondering if it might be related to this:

microsoft/TypeScript#33503

@sheetalkamat
Copy link
Contributor

It is... I will send a patch for this as we want to be using new apis if there.

@johnnyreilly
Copy link
Member Author

Awesome - thanks!

@snyk-bot snyk-bot mentioned this pull request Oct 27, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sheetalkamat sheetalkamat sheetalkamat left review comments

@andrewbranch andrewbranch Awaiting requested review from andrewbranch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@johnnyreilly @sheetalkamat