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

chore: Pnpm migration #627

Merged
merged 30 commits into from Oct 18, 2021
Merged

Conversation

semoal
Copy link
Contributor

@semoal semoal commented Oct 5, 2021

It contains the Lerna pull request changes, updated Jest and fixed some eslint warns

@semoal
Copy link
Contributor Author

semoal commented Oct 5, 2021

Not sure why it fails on Windows:

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      303 |   // FIXME: --props don't work well on windows, this is an edge case for example
      304 |   // In this case we should warn the user about it that they should pass a file path instead
    > 305 |   expect(fs.existsSync(outputPath)).toBe(process.platform !== "win32");
          |                                     ^
      306 |
      307 |   if (process.platform !== "win32") {
      308 |     const info = await execa("ffprobe", [outputPath]);

But the migration is done, you can try on your local machine and you'll see how faster is now to develop and contribute :)

It was 11 minutes of CI on Node 12, and now 7 minutes and the cache of the dependencies store is not ready yet ^^

@semoal semoal force-pushed the semoal/pnpm-migration branch 2 times, most recently from 6c76ee3 to 68db739 Compare October 5, 2021 17:13
@JonnyBurger
Copy link
Member

Thanks a lot! This looks super sick, I love seeing the build times go down significantly!

I will award the bounty now with a bonus, and there is no further action needed on your side for now, but I will take a bit to merge it, to adjust:

  • Ensure it works well also on Lambda branch
  • The contributing docs (and maybe rerecord the how to contribute to Remotion video)
  • At least dig down the windows issue, should not be a big dealbreaker but I'd like to understand it

Looking to do that in the next few days 🥳

@JonnyBurger
Copy link
Member

The docs (npm start / npm run build-docs) don't work for me. Some nasty dependency change in the lockfiles 🙄

I'm also ok if we refactor the affected code snippets to avoid this breakage which doensn't happen for the first time.

image

@semoal
Copy link
Contributor Author

semoal commented Oct 6, 2021

The docs (npm start / npm run build-docs) don't work for me. Some nasty dependency change in the lockfiles 🙄

I'm also ok if we refactor the affected code snippets to avoid this breakage which doensn't happen for the first time.

image

Did you try to run:

rm -rf packages/**/node_modules && rm -rf node_modules

A clean install should fix all these errors, I've tested on my local machine and apparently Shiki and Docusaurus worked fine :)

@JonnyBurger
Copy link
Member

Unfortunately still doesn't work :(

I added the docs build to CI to see if it works here.

Another thing to investigate: Can we make prereleases like ^2.5.0-alpha.4239h0 (https://remotion.dev/docs/prereleases)? 🤔

@semoal
Copy link
Contributor Author

semoal commented Oct 6, 2021

Unfortunately still doesn't work :(

I added the docs build to CI to see if it works here.

Another thing to investigate: Can we make prereleases like ^2.5.0-alpha.4239h0 (remotion.dev/docs/prereleases)? 🤔

Hm I've tried locally with Node 12 and Node 14 and worked fine, going to try it now with 16 if that's the issue.

About the prerelease, pnpm is a package manager and doesn't contain any preconfigured setup to make releases but we can introduce back Lerna and just use Lerna to release (I already do that on my projects so is not a problem)

@JonnyBurger
Copy link
Member

You are right, easy enough, I just added back Lerna and my npx lerna publish command works again! (Better now actually with v4 since the versions are strictly increasing with every prerelease so you don't accidentially install the wrong version).

I also triggered a docs build, let's see if it's a bug I only encounter locally!

@JonnyBurger
Copy link
Member

I'll continue on this PR this weekend probably! Sorry for the delay, a bit of a backlog has been adding up :)

@JonnyBurger
Copy link
Member

So everything is green except the docs don't build: https://github.com/semoal/remotion/runs/3918744826?check_suite_focus=true

Any ideas? It's super hard to debug from that information 🤔 in the worst case we have to just try and find the problem by slowing removing twoslash directives until we find the one that breaks.

What makes it even weirder, only happens in CI not locally 😈

@semoal
Copy link
Contributor Author

semoal commented Oct 18, 2021

I'll take a look today Jonny to the docs issue, looks a Twoslash issue resolving symlinked dependencies.

@JonnyBurger
Copy link
Member

🟢🟢🟢🟢 Greeeeen!

Will make some changes to the contributing guide, then ship it!

@JonnyBurger JonnyBurger merged commit 443b915 into remotion-dev:main Oct 18, 2021
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.

Update to Lerna 4 Cannot update to Docusaurus 2.0.0 beta 3 with twoslash enabled
2 participants