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

Use mono_repo for package:web #288

Merged
merged 10 commits into from
Aug 23, 2024
Merged

Use mono_repo for package:web #288

merged 10 commits into from
Aug 23, 2024

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented Aug 23, 2024

Splits package:web into two packages:

  • package:web
  • package:web_generator (not published)

The latter contains our tooling to generate the former, and used to be the tool directory.

  • Refactors code to point to new paths (and using Platform.script.resolve so that the bin/ files can be run from anywhere instead of within that directory)
  • Moves directories like third_party and tests to their new appropriate locations
  • Moves CI configurations to mono_repo.yaml and mono_pkg.yaml files and is now generated as dart.yml (used to be build.yaml)
  • Updates READMEs to reflect the package split
  • Adds new LICENSE, pubspec.yaml, and CHANGELOG files
  • Removes JSArrayExtension which is no longer needed.

Refactors code to point to new paths and moves directories like third_party to their appropriate locations. Moves the CI configurations to mono_repo.yaml and mono_pkg.yaml files, and is now generated as dart.yml.

Splits package:web into two packages:
- package:web
- package:web_generator (not published)

The latter contains our tooling to generate the former,
and used to be the tool directory.

Refactors code to point to new paths and moves directories
like third_party to their appropriate locations. Moves the
CI configurations to mono_repo.yaml and mono_pkg.yaml files,
and is now generated as dart.yml.
@kevmoo
Copy link
Member

kevmoo commented Aug 23, 2024

But...why? What does this get us?

@srujzs
Copy link
Contributor Author

srujzs commented Aug 23, 2024

This came up with analysis of an external PR. Analysis with the latest dev version has started failing due to the generator using JSArray.length and JSArray.[]. Since those members didn't exist until recently in dart:js_interop, we were using the declarations in util.dart. Once those members were added to dart:js_interop, it started using the definitions in the SDK instead implicitly because type members have higher precedence than extension members. However, when we added @Since annotations, analysis started to fail because the minimum SDK version of this package was not >=3.6. This will start turning CI red.

The solutions were:

  • Update the minimum SDK constraint. This may be okay, but it doesn't make sense to require a later SDK version even though none of the code users can actually use is impacted. This probably also isn't the last time we'll need to do this.
  • Disable analysis for the generator (temporary, but not tractable long term).
  • Use extension overrides for every single usage of those JSArray members (bad considering there are like ~25 usages).
  • Separate the generation code from the interop definitions (this PR).

As a bonus, if we wanted, we could publish the generator as a separate package so users can use it to generate definitions that we don't emit in this package. This was discussed in #145 before, but is not really the primary goal of this PR.

README.md Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
web_generator/third_party/mdn/LICENSE Outdated Show resolved Hide resolved
@kevmoo kevmoo changed the title [WIP] Use mono_repo for package:web Use mono_repo for package:web Aug 23, 2024
Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

Thanks!

@srujzs srujzs merged commit 46d416f into dart-lang:main Aug 23, 2024
15 checks passed
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.

3 participants