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

Migrate to ESM #335

Closed
wants to merge 17 commits into from
Closed

Migrate to ESM #335

wants to merge 17 commits into from

Conversation

kytta
Copy link

@kytta kytta commented Sep 11, 2023

  • Install Vitest
  • Migrate sub-packages
    • dual-publish
    • esbuild
      • figure out why sizes differ way too much (231 405 bytes)
    • esbuild-why
    • file
    • preset-app
    • preset-big-lib
    • preset-small-lib
    • size-limit
      • third party plugin used for testing uses imports incorrectly (without an extension)
    • time
    • webpack
    • webpack-css
      • figure out why it doesn't seem to bundle CSS at all (1450 70 bytes)
    • webpack-why
  • update ESLint config
  • test JS config with ESM

@kytta
Copy link
Author

kytta commented Sep 12, 2023

Things I'm stuck on at the moment (just to keep records):

  1. Rewriting require into import doesn't work for something more complicated, like ESBuild and webpack plugins. I haven't figured out the whole errors yet.
  2. When the plugins do work, they tend to provide different size results. For example, files compressed with ESBuild tend to be bigger than before, but I haven't figured out why yet. I've tried converting test files into both ESM and CJS, but none worked as expected. As such, snapshot tests fail

Also:

  • Vitest doesn't have a plausible alternative to jest.setTimeout(15000); we have to set a timeout for every it() call or for every test per run via a config file, but not per test file. Instead of plastering it on every test, I only apply it to the ones that actually need more time

@ai
Copy link
Owner

ai commented Sep 12, 2023

Vitest doesn't have a plausible alternative to jest.setTimeout(15000); we have to set a timeout for every it() call or for every test per run via a config file, but not per test file. Instead of plastering it on every test, I only apply it to the ones that actually need more time

Try to remove it. Maybe we don’t need it anymore (by increasing performance with new Node.js and esbuild).

Rewriting require into import doesn't work for something more complicated, like ESBuild and webpack plugins. I haven't figured out the whole errors yet.

Need some example to understand what exactly problems do you have

When the plugins do work, they tend to provide different size results. For example, files compressed with ESBuild tend to be bigger than before, but I haven't figured out why yet. I've tried converting test files into both ESM and CJS, but none worked as expected. As such, snapshot tests fail

If it is only a few bytes, it could happen just because of dependencies update (new esbuild compress a little different). Just updates tests and snapshots.

@kytta
Copy link
Author

kytta commented Sep 12, 2023

Alright, this is mostly done now, only couple of bugs left.

Rewriting require into import doesn't work for something more complicated, like ESBuild and webpack plugins. I haven't figured out the whole errors yet.

Need some example to understand what exactly problems do you have

Currently stuck on webpack-css with the following:

AssertionError: expected 70 to be greater than 1450
 ❯ packages/webpack-css/test/index.test.js:41:33
     39|   }
     40|   await run(config)
     41|   expect(config.checks[0].size).toBeGreaterThan(1450)
       |                                 ^
     42|   expect(config.checks[0].size).toBeLessThan(2400)
     43| })

Seems like the non-JS requires inside nonjs.js get completely ignored, as the file size stays the same. I wonder if it's because I still require() them instead of using dynamic imports — haven't had time to test this out yet.

When the plugins do work, they tend to provide different size results.

If it is only a few bytes, it could happen just because of dependencies update (new esbuild compress a little different). Just updates tests and snapshots.

For the "supports ignore" test on esbuild, it doesn't seem to, well, support ignore, as the bundle size is way bigger than expected

@ai
Copy link
Owner

ai commented Sep 12, 2023

AssertionError: expected 70 to be greater than 1450

70 bytes is nothing. Just update the size.

For the "supports ignore" test on esbuild, it doesn't seem to, well, support ignore, as the bundle size is way bigger than expected

This is more important bug. But with the current data I can’t give you any advice rather than keep debuging.

@kytta
Copy link
Author

kytta commented Sep 12, 2023

AssertionError: expected 70 to be greater than 1450

70 bytes is nothing. Just update the size.

How so? It is expected that the final webpack bundle is in the 1450–2400 range, but it's just 70 (which coincidentally matches the size of the unbundled file), so 2–3 orders of magnitude smaller.

@ai
Copy link
Owner

ai commented Sep 12, 2023

It is expected that the final webpack bundle is in the 1450–2400 range, but it's just 70

Got it! Yes, it is not expected (I thought that it is 70 less, not equal to 70).

@ai
Copy link
Owner

ai commented Oct 23, 2023

Closing for #338

@ai ai closed this Oct 23, 2023
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