Skip to content

Update dependencies + test on Node>10 compatible dependencies #300

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

Merged
merged 32 commits into from
Jan 9, 2021

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Sep 7, 2020

  • This updates all the dependencies

  • Tests threads-js on Node 10-14 via GitHub Actions

- rollup-script and ava-script are used to make it possible to run the tests on old Node versions.

@aminya
Copy link
Contributor Author

aminya commented Sep 7, 2020

Bundle command fails on Node 8 because TypeScript generates import in the ESM output, and you are trying to bundle the Code from the output of TypeScript. Instead, you can use a Rollup-TypeScript plugin to bundle the code from the beginning.

@aminya
Copy link
Contributor Author

aminya commented Sep 13, 2020

I reverted using Parcel because of parcel-bundler/parcel#5135

@andywer
Copy link
Owner

andywer commented Sep 13, 2020

Side note: You definitely need to keep rollup. Not necessarily for bundling, but just to make sure that our ES module output can be consumed by rollup, since we had issues with rollup in the past 😉

@aminya
Copy link
Contributor Author

aminya commented Sep 16, 2020

Well currently, using the new Rollup people cannot build threads on Node 8.

Do you want us to use a Pre Node 10 rollup script for that?

As you see in https://travis-ci.org/github/andywer/threads.js/jobs/726678405#L272, Rollup now uses dynamic import which is not defined in very old Node.

If we have two Rollup scripts, one for the old Node and one for new Node, we can make sure that this builds across all systems.

aminya referenced this pull request in andywer/puppet-run Sep 16, 2020
* use del instead of rimraf

* Update package-lock.json
@aminya
Copy link
Contributor Author

aminya commented Sep 16, 2020

We can't install two rollups at the same time. Their bin file conflicts!

Using aliases does not help.

    "rollup-old": "npm:rollup@^1",
    "rollup": "^2.26.10",

@andywer I think you should decide that you want to be compatible with the old Rollup or the new one (which probably everyone uses).

@aminya
Copy link
Contributor Author

aminya commented Sep 16, 2020

We can use a custom script to call Rollup!

@aminya aminya force-pushed the update-deps branch 4 times, most recently from 28267cb to ad2d8a8 Compare September 16, 2020 09:31
@aminya aminya changed the title Update dependencies Update dependencies + Fix old Rollup Sep 16, 2020
@aminya
Copy link
Contributor Author

aminya commented Sep 16, 2020

The old Node is pretty stupid 💢 It does not even parse catch {}.

@aminya aminya changed the title Update dependencies + Fix old Rollup Update dependencies + test on Node>10 compatible dependencies Sep 16, 2020
@andywer
Copy link
Owner

andywer commented Sep 16, 2020

@aminya Thanks so much for your efforts!

The old Node is pretty stupid 💢 It does not even parse catch {}.

Yeah, the blank catch {} syntax hasn't been around for that long. You can of course always write catch (error) {} and just not use the error, although you might have to tell the linter that this is fine in that case.

I am sorry that I can only contribute so little time at the moment, but I will try to help you as good as I can! That's obviously an important PR.

npm install -g npm-check-updates
ncu -u -x mocha
@aminya aminya marked this pull request as ready for review October 27, 2020 04:27
npm sometimes fail to run prebuild before build. Using clean makes it 
explicit
@aminya
Copy link
Contributor Author

aminya commented Oct 27, 2020

This is ready to go 🚀

"prebuild": "rimraf dist/ dist-esm/",
"build": "run-p build:cjs build:es",
"clean": "rimraf dist/ dist-esm/",
"build": "npm run clean && npm run build:cjs && npm run build:es",
Copy link
Owner

Choose a reason for hiding this comment

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

@aminya Any particular reason why you don't want to run it in parallel anymore? It will take much longer this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm-run-all is unmaintained. It randomly fails on Windows. That should not be a problem though. For development, we can add a dev command which uses TypeScript's watch feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added dev command in c09017e

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough, but how about concurrently then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can try that.

@aminya
Copy link
Contributor Author

aminya commented Oct 28, 2020

Here is the CI run for the record: https://github.com/aminya/threads.js/actions/runs/333498686

Not sure why it is not shown here.

@andywer
Copy link
Owner

andywer commented Oct 28, 2020

@aminya Btw, I really appreciate the time and effort you put into this! Especially since I don't have much time right now. Great job 👍

@aminya
Copy link
Contributor Author

aminya commented Nov 1, 2020

You're welcome 👍🏻 😉

@andywer
Copy link
Owner

andywer commented Nov 2, 2020

@aminya There is one thing about the GitHub actions workflow that we still need to fix: When the test fails with the retry action, we do not get to see the original error anymore: see here, for instance.

But I guess we should not auto-retry flaky tests in the first place. Let's check why they are flaky and fix them. It's also a bit strange, since they weren't flaky before – I could run them reliably on Travis CI and locally 🤔

@aminya
Copy link
Contributor Author

aminya commented Nov 2, 2020

@aminya There is one thing about the GitHub actions workflow that we still need to fix: When the test fails with the retry action, we do not get to see the original error anymore: see here, for instance.

But I guess we should not auto-retry flaky tests in the first place. Let's check why they are flaky and fix them. It's also a bit strange, since they weren't flaky before – I could run them reliably on Travis CI and locally 🤔

The tests of the package itself are fine. The test that bundles the code using Rollup sometimes timeouts! I tried to increase its timeout number in this commit, but that does not seem to make a difference 🤷‍♂️ Maybe I am doing it wrong.

@andywer
Copy link
Owner

andywer commented Nov 2, 2020

The test that bundles the code using Rollup sometimes timeouts!

Ohh, I see… Maybe we have a race condition in the rollup testing code. I can look into it later – some additional logging might give us a hint.

@aminya
Copy link
Contributor Author

aminya commented Jan 6, 2021

Bump

@andywer
Copy link
Owner

andywer commented Jan 6, 2021

Thanks for the reminder! No time to look into it today, though, I fear… Maybe I get around looking into it until weekend.

@aminya
Copy link
Contributor Author

aminya commented Jan 9, 2021

Thanks. Hopefully, we can merge this on the weekend. This was ready and working. But some stuff has merged into master.

@andywer andywer merged commit 4d37802 into andywer:master Jan 9, 2021
@andywer
Copy link
Owner

andywer commented Jan 9, 2021

Merged 😉

@andywer
Copy link
Owner

andywer commented Jan 9, 2021

@aminya Thanks again! Btw, do you need an update published urgently or were you just waiting for the dev dependencies to get fixed?

@aminya
Copy link
Contributor Author

aminya commented Jan 9, 2021

@andywer Thanks! This was blocking #290. Now I can merge that PR.

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 this pull request may close these issues.

2 participants