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

[bundling] Change bundling #106

Merged
merged 22 commits into from
Nov 14, 2024
Merged

[bundling] Change bundling #106

merged 22 commits into from
Nov 14, 2024

Conversation

yoannmoinet
Copy link
Member

@yoannmoinet yoannmoinet commented Oct 25, 2024

What and why?

We should not bundle external dependencies in our published packages.
We still need to bundle though, because we have shared internal dependencies cross workspaces.

Also update our dependencies.

How?

  1. Dedupe dependencies.
  2. Upgrade rollup (+ plugins), vite, unplugin and esbuild.
  3. Remove fs-extra and replace it with helpers and native fs.
  4. Update Jest's setup to avoid collision with mocks (of fs in @dd/core/helpers specifically).
    a. Move the setup files in a dedicated folder, and apply an eslint rule on it to avoid any internal imports in it.
  5. Fix collisions of the injected file in esbuild.
  6. Explicitly list dependencies of the packages we publish (based on what they internally depend on).
  7. Add a new step in yarn cli integrity to verify that we have all the necessary dependencies in the published package.
  8. Add --cleanup=0 and --build=1 flags for running tests.
    a. Jest actually does not support custom empty parameters and threw when I tried to add --build. It didn't throw with --no-cleanup because it's using yargs that automatically changed it to --cleanup=0 behind the scene, which passed Jest's validation of accepting custom params with values. So I changed it to --cleanup=0 for consistency with the new --build=1.
    b. --build=1 will build all the bundlers' packages necessary prior to the test run.

@@ -20,7 +20,12 @@ export const BUNDLER_VERSIONS: Record<BundlerFullName, string> = {
webpack5: require('webpack5').version,
};

export const NO_CLEANUP = process.argv.includes('--no-cleanup');
export const NO_CLEANUP = process.argv.includes('--cleanup=0');
Copy link
Member Author

@yoannmoinet yoannmoinet Oct 28, 2024

Choose a reason for hiding this comment

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

Apparently Jest doesn't allow custom flags without values.
--no-cleanup was automatically transformed into --cleanup=0 by Jest's dependency yargs so it passed through the validation because it has a value in the end.

So I transformed it to --cleanup=0 to have continuity with --build=1 because simply --build wasn't possible, Jest was flagging it.

@yoannmoinet yoannmoinet changed the title Yoann/change bundling [Bundling] Change bundling Oct 29, 2024
Comment on lines +99 to +101
// @ts-expect-error PQueue's default isn't typed.
const Queue = PQueue.default ? PQueue.default : PQueue;
const queue = new Queue({ concurrency: options.maxConcurrency });
Copy link
Member Author

@yoannmoinet yoannmoinet Oct 29, 2024

Choose a reason for hiding this comment

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

This is the usual mess of highly opinionated Sindre Sorhus' packages.
And I can't use the latest version (yet) because it's ESM only.

Copy link
Member

Choose a reason for hiding this comment

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

bummer :(

exports.default = PQueue;

@yoannmoinet yoannmoinet marked this pull request as ready for review October 29, 2024 11:21
@yoannmoinet yoannmoinet requested a review from a team as a code owner October 29, 2024 11:21
@yoannmoinet yoannmoinet requested review from elbywan and removed request for a team October 29, 2024 11:21
await fs.remove(target);
await fs.mkdir(target);
await rm(target);
await fs.mkdir(target, { recursive: true });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: shouldn't you make a helper for this too?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fair.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(no need to be done in this PR)

@yoannmoinet yoannmoinet changed the title [Bundling] Change bundling [bundling] Change bundling Oct 29, 2024
Comment on lines +99 to +101
// @ts-expect-error PQueue's default isn't typed.
const Queue = PQueue.default ? PQueue.default : PQueue;
const queue = new Queue({ concurrency: options.maxConcurrency });
Copy link
Member

Choose a reason for hiding this comment

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

bummer :(

exports.default = PQueue;

packages/tools/src/commands/integrity/dependencies.ts Outdated Show resolved Hide resolved
@yoannmoinet yoannmoinet merged commit 19f8316 into master Nov 14, 2024
5 checks passed
@yoannmoinet yoannmoinet deleted the yoann/change-bundling branch November 14, 2024 16:09
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