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

Convert @embroider/addon-dev to ESM #1766

Open
wants to merge 13 commits into
base: stable
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Jan 23, 2024

  • Convert addon-dev to ESM ( 👈 you are here)
    • ended up being required due to how tsc forbids ESM (even await import) in CJS-output (and we compile with tsc)
      • this came up because fix-bad-declaration-output is ESM only
    • Compiled with SWC
    • Do we need to generate types for everything? I saw only types for template-colocation-plugin were exported
      • This'll be hard until the rest of the embroider packages are ESM, and we have a better way to provide "global types" for dependencies. For example, @embroider/core requres types for NestedHooks, beforeAll, etc, and those types don't exist in addon-dev -- nor do they make sense to exist. Maybe it makes more sense to get rid of those globals and define the types we need to use for those functions.
  • Add addon.glint() utility to help out in the v2 addon blueprint
    • remove double package.json#scripts entries managed by concurrently
    • better CLI output (two tools don't try to clear each other)
    • work-around a couple problems with Glint (gts extensions, etc)
  • Remove the sample files
    • with the package becoming type=module, the extensions needed to change / be consistent with type=module schemeing. I didn't see they were actually used though, so they've been removed.

@NullVoxPopuli NullVoxPopuli marked this pull request as draft January 23, 2024 21:40
Copy link
Collaborator

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

Looks like this is blocked on @embroider/addon-dev needing to be ESM.

I think you could use dynamic import inside closeBundle to not be blocked on esm conversion:

const { execaCommand } = await import('execa');

packages/addon-dev/src/rollup-glint-plugin.ts Outdated Show resolved Hide resolved
@NullVoxPopuli
Copy link
Collaborator Author

I think you could use dynamic import inside closeBundle to not be blocked on esm conversion:

oh! that's a good idea! I always forget that's a thing. been so long since I've actually had to worry about cjs 😅

@NullVoxPopuli
Copy link
Collaborator Author

Nevermind -- our build setup is removing all imports...........
image

@NullVoxPopuli NullVoxPopuli force-pushed the provide-glint-utility-rollup-plugin branch from 8484ff9 to 3d8be4c Compare February 9, 2024 21:11
@NullVoxPopuli
Copy link
Collaborator Author

Been trying a few different compilation techniques, and it seems that there is a lot of:

[!] SyntaxError: Named export 'readJsonSync' not found. The requested module 'fs-extra' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'fs-extra';
const { readJsonSync, writeJsonSync } = pkg;

style of errors, including from @embroider/core

@NullVoxPopuli NullVoxPopuli changed the title Provide Glint utility plugin for @embroider/addon-dev Convert @embroider/addon-dev to ESM Feb 9, 2024
@NullVoxPopuli
Copy link
Collaborator Author

woah, it's green

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review February 12, 2024 17:02
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