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

feat(cli): support dynamic imports from cjs #22638

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

sennyeya
Copy link
Contributor

@sennyeya sennyeya commented Feb 1, 2024

Hey, I just made a Pull Request!

might fix #8242?

Context: I ran into the ESM module support issue while testing out ink + a new Backstage CLI. It would be great to start the ball rolling on adding support and this seems like the first blocker.

This PR adds a new plugin that handles rollup's resolveDynamicPlugin and renderDynamicPlugin methods. Currently, I enabled it just for non-relative files as the bundler doesn't pick up relative .mjs files directly. Given the context of where this would be used, I doubt that that is a necessary usecase as well. I did see that the commonjs plugin is set up to run against just node_modules,

       commonjs({
          include: /node_modules/,
          exclude: [/\/[^/]+\.(?:stories|test)\.[^/]+$/],
        }),

Not sure if that impacts this work. From what I can tell, the resolutions for the dynamic imports work despite this, but it may require further investigation. Interestingly, I only needed to change the build config, and was able to leave the webpack config untouched.

Testing plan: I'm not quite sure what else should be tested here. I ran both the build and start commands with dynamic imports and they worked as expected (like the snapshot in the rollup test). Would love ideas here.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@sennyeya sennyeya requested review from a team as code owners February 1, 2024 04:38
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Feb 1, 2024

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

  • @backstage/backend-dynamic-feature-service
  • @backstage/repo-tools
  • @backstage/plugin-app-backend
  • @backstage/plugin-auth-backend-module-guest-provider
  • @backstage/plugin-catalog-backend-module-unprocessed
  • @backstage/plugin-catalog-backend

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/backend-dynamic-feature-service packages/backend-dynamic-feature-service none v0.2.10-next.2
example-backend packages/backend none v0.0.26-next.1
@backstage/cli packages/cli minor v0.26.5-next.1
@backstage/repo-tools packages/repo-tools none v0.9.0-next.2
@backstage/plugin-app-backend plugins/app-backend none v0.3.66-next.1
@backstage/plugin-auth-backend-module-guest-provider plugins/auth-backend-module-guest-provider none v0.1.4-next.1
@backstage/plugin-catalog-backend-module-unprocessed plugins/catalog-backend-module-unprocessed none v0.4.5-next.2
@backstage/plugin-catalog-backend plugins/catalog-backend none v1.22.0-next.2

Copy link
Contributor

github-actions bot commented Feb 1, 2024

Uffizzi Ephemeral Environment - Virtual Cluster

☁️ deploying cluster pr-22638

⚙️ Updating now by workflow run 8608047205.

Download the Uffizzi CLI to interact with the upcoming virtual cluster
https://docs.uffizzi.com/install

@sennyeya
Copy link
Contributor Author

sennyeya commented Feb 4, 2024

The E2E test failure is definitely related to this work,

[yarn new --select plugin --option id=test].err: TypeError: actionFunc is not a function

I'll investigate this week.

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label Feb 11, 2024
@sennyeya sennyeya requested a review from a team as a code owner February 12, 2024 01:19
@github-actions github-actions bot added the area:techdocs Related to the TechDocs Project Area label Feb 12, 2024
@github-actions github-actions bot removed the stale label Feb 12, 2024
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉 👏

Big step towards module support. Gonna play it safe and ship this after the next main-line.

packages/cli/src/commands/index.ts Outdated Show resolved Hide resolved
@sennyeya sennyeya requested a review from Rugvip February 12, 2024 13:32
@Rugvip Rugvip added the merge-after-release This is a bit too scary to merge until after the next release label Feb 13, 2024
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label Feb 20, 2024
@github-actions github-actions bot closed this Feb 25, 2024
@freben
Copy link
Member

freben commented Feb 25, 2024

@Rugvip this was still of interest right

@freben freben reopened this Feb 25, 2024
@freben freben removed the stale label Feb 25, 2024
@Rugvip
Copy link
Member

Rugvip commented Feb 25, 2024

@freben yep 👍

Copy link
Contributor

github-actions bot commented Mar 3, 2024

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label Mar 3, 2024
@freben freben removed the stale label Mar 3, 2024
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label Mar 10, 2024
@freben freben removed the stale label Mar 10, 2024
Copy link
Contributor

github-actions bot commented Apr 8, 2024

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label Apr 8, 2024
@freben freben removed the stale label Apr 8, 2024
@sennyeya sennyeya requested a review from a team as a code owner April 8, 2024 23:34
@sennyeya sennyeya requested a review from Edje-C April 8, 2024 23:34
@github-actions github-actions bot added the area:catalog Related to the Catalog Project Area label Apr 8, 2024
@aramissennyeydd
Copy link
Contributor

aramissennyeydd commented Apr 8, 2024

@Rugvip (sorry for the late response here) I think I misread the rollup option and assumed it was doing both imports. When I was initially working on this, I noticed some fairly odd behavior around relative dynamic imports with rollup. I just pushed a commit with such a broken state in plugins/catalog-backend and packages/backend-next. The output of plugins/catalog-backend doesn't respect the dynamic import and in fact converts my MJS to CJS, packages/backend-next explodes with a ERR_UNSUPPORTED_DIR_IMPORT error. I'm pretty squarely out of my depth here 🙃, so not too sure what good next steps are here. Was thinking of opening issues outlining what I was running into in both projects.

@Rugvip
Copy link
Member

Rugvip commented Apr 14, 2024

@aramissennyeydd alright, thank you for the update 👍

Could let it sit for the time being if you want, I might poke around at this some more at some point too.

Regarding Webpack we'll get rid of that in not too long for backend builds, it'll be part of deprecating support for the old backend system.

@aramissennyeydd
Copy link
Contributor

Ah, sorry, had meant rollup 😅 . I'll try and wade through the rollup/webpack/swc miasma next week and see if I can come up with anything. The mismatch between documented options and output is bugging me a little.

Copy link
Contributor

github-actions bot commented May 5, 2024

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

aramissennyeydd and others added 8 commits May 10, 2024 21:53
Signed-off-by: Aramis Sennyey <159921952+aramissennyeydd@users.noreply.github.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: Aramis Sennyey <159921952+aramissennyeydd@users.noreply.github.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: Aramis Sennyey <159921952+aramissennyeydd@users.noreply.github.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: Aramis Sennyey <159921952+aramissennyeydd@users.noreply.github.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
@aramissennyeydd
Copy link
Contributor

@Rugvip I think I'm getting closer, the SWC maintainer pointed me to an ignoreDynamic option that keeps the imports as is - which breaks some functionality on our end 😅 . Namely the CLI importing pattern and allowing relative dynamic imports in the backend definition,

lazy(() => import('./start').then(m=>m.command))

and

backend.add(import('./authGithubProvider'))

Those were both being compiled to require statements with the correct file mappings (not necessarily the same as the source files). I don't think we can have it both ways of using imports and expecting some compiled and some uncompiled code.

It also means we'll have to adjust package definitions to better support ESM resolution, TS is not well behaved here (TypeStrong/ts-node#1997). The error on our side looks like,

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /Users/aramis.sennyey/Projects/backstage/backstage/plugins/catalog-backend-module-scaffolder-entity-model/src/index.ts

I did to a few PRs here, basically adjusting the main field to point to a dist/file.cjs.js file instead of the TS allows us to use dynamic imports. Unfortunately, that would end up being a breaking change as you couldn't use any plugins with dynamic import that don't have this correctly defined. Open to ideas here, I know there's a similar in between step of publishConfig which points to these files as you would expect.

@github-actions github-actions bot removed the stale label May 11, 2024
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label May 29, 2024
@aramissennyeydd aramissennyeydd removed stale merge-after-release This is a bit too scary to merge until after the next release labels May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:catalog Related to the Catalog Project Area auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: Support dynamic imports in backend code
5 participants