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: migrate fully to Yarn #3155

Merged
merged 6 commits into from
Nov 10, 2021
Merged

chore: migrate fully to Yarn #3155

merged 6 commits into from
Nov 10, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Nov 5, 2021

Depends on flarum/flarum-webpack-config#14

Changes proposed in this pull request:

This fully migrates core to Yarn for JS package management. This includes:

  • Using Yarn berry (not v1)
    • "berry" is the version name for everything Yarn v2 and later. Yarn v2 is not used by default on any system running Yarn due to how vastly different it is.
  • Using PnP for dependency management and package resolving

We've discussed switching to Yarn for a long time now, and we do use it in all of our newer JS repositories, including Flarum CLI and Webpack Config.

For more info about PnP, please see the Webpack Config PR.

Reviewers should focus on:

Everything, honestly.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@davwheat davwheat self-assigned this Nov 5, 2021
js/.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

I strongly dislike committing generated boilerplate code unless there are tools to keep it up to date, so this sdk thing worries me. What happens when that becomes out of date / there's a new version? Will there be some kind of warning / error?

}

// Defer to the real prettier/index.js your application uses
module.exports = absRequire(`prettier/index.js`);
Copy link
Member

Choose a reason for hiding this comment

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

Will we need this in every single repo that uses prettier?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it's only needed for Prettier and TS integration with VSCode and other IDEs.

Imo I think this is worth committing because it creates a working out of the box dev environment.

The code can be updated automatically vwith yarn dlx @yarnpkg/sdks, though the VS Code config file needs to be altered slightly in order to play nicely with our js folder being a subdirectory.

https://yarnpkg.com/getting-started/editor-sdks

Copy link
Member

Choose a reason for hiding this comment

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

My concern is keeping this up to date across our (many) repos. What manual changes are needed, and could we automate them somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

By default, it generates a vscode config file in the JS folder. We just move that out, and prepend js/ to any paths needed.

Looking now, I can't actually see the vscode config file in this PR... I need to add that.

Copy link
Member

Choose a reason for hiding this comment

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

By default, it generates a vscode config file in the JS folder. We just move that out, and prepend js/ to any paths needed.

Looking now, I can't actually see the vscode config file in this PR... I need to add that.

I suppose we could add something to flarum-cli. But I wonder if the huge pile of boilerplate we need to keep track of is worth it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's one of the things that won't be a 'huge pile' when we go monorepo eventually. Our root workspace could have Prettier installed with this boilerplate, and everything would work fine in subdirectories.

Copy link
Member

@askvortsov1 askvortsov1 Nov 8, 2021

Choose a reason for hiding this comment

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

It's one of the things that won't be a 'huge pile' when we go monorepo eventually. Our root workspace could have Prettier installed with this boilerplate, and everything would work fine in subdirectories.

It's not just us: monorepo or not, we can't sacrifice developer experience for third party devs.

I know these comments aren't particularly constructive. But I guess my point is that I'm not sure that Yarn 2 is worth it atm. It seems like most major players are still in the npm / yarn 1 system, and that npm itself is getting a lot better and more scalable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just a note that these changes you're referring to are specifically for Yarn PnP support rather than Yarn 2 or Yarn 3.

Reverting 9781c8c will let us use Yarn 3 without PnP using the legacy node_modules linker.

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, if we want to commit entirely to Yarn, I would prefer switching to Yarn 3 without PnP, and then reconsider PnP at a later time. Incremental steps!

@davwheat
Copy link
Member Author

davwheat commented Nov 8, 2021

There should be a message stating it's out of date if you're referring to committing Yarn itself. The reasoning behind it is to prevent inconsistencies where people use different versions of package managers, similar to inconsistencies we had when NPM 7 released and they introduced a new lockfile format.

@davwheat davwheat merged commit 563d40d into master Nov 10, 2021
@davwheat davwheat deleted the dw/yarn-migration branch November 10, 2021 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants