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

Refactor: Move code of lib/manager-api into ui/manager #23389

Closed
wants to merge 11 commits into from

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jul 10, 2023

hyper experimental: move the actual code into ui/manager but keep lib/manager-api a real package with types.

Long term strategy: move all code into a single package.

This has upside of making building manager-api unnecessary during storybookmaintenance.
the downside is that building manager is slower.

But it's a step into the right direction, I think.

Importantly this does not mean manager-api gets deprecated as a package.
It's still a package users depend on and use.

What's novel about this approach is that it doesn't need a builder to globalize manager-api! It's builder independent, because the package itself gets the implementation from the global scope.

This has a big downside:
It will only work within the browser, and only when the prebundles code is pre-injected.
You can see the effects of this in some of the tests I had to adjust.

So this is a proof of concept of the idea that we could make packages be externalised without needing to set this up in builders.

@ndelangen ndelangen self-assigned this Jul 10, 2023
@ndelangen ndelangen added ci: do not merge ci:daily Run the CI jobs that normally run in the daily job. labels Jul 10, 2023
@socket-security
Copy link

socket-security bot commented Jul 10, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

@ndelangen ndelangen changed the base branch from release/7.2 to norbert/remove-references-to-api July 10, 2023 19:49
@ndelangen ndelangen changed the base branch from norbert/remove-references-to-api to release/7.2 July 10, 2023 19:49
@ndelangen ndelangen requested a review from JReinhold July 10, 2023 20:42
@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Jul 10, 2023
@JReinhold
Copy link
Contributor

This is interesting, looks good. Is the long term plan that we'd then remove the manager-api package completely in a major release?

I don't quite understand what you mean by the following:

It will only work within the browser, and only when the prebundles code is pre-injected.

@ndelangen
Copy link
Member Author

@JReinhold what it means is:

  • the user imports @storybook/manager-api they depend on it in their package.json like normal.
  • the actual runtime content of the package actually reads from the global namespace though!
  • the builder doesn't need to be configured to do this at all, it jut bundles code like it always does, it does not externalise anything.
  • The prebundled code executes before other code, and thus the globals are there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:daily Run the CI jobs that normally run in the daily job. ci: do not merge maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants