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

[RFC & POC] Hiding Wasp's private API from users #1922

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sodic
Copy link
Contributor

@sodic sodic commented Mar 26, 2024

1 Context

The plan for Wasp 0.12.0 was:

  • Move Wasp's public API into the Wasp SDK (i.e., node_modules/wasp) and expose it to the user.
  • Keep Wasp's private API (i.e., under-the-hood functionalities Wasp relies on) inside the framework code (i.e., .wasp/out{server,web-app}), hiding it from the user.

Unfortunately, due to tight coupling between private and public functionalities, we've ended up having to move a large chunk of Wasp's private API into the SDK:

What we wanted: What we have:

2 The private API problem

The framework code (i.e., the runtime) importing private API from the SDK is not a problem in and of itself. However, it does requires the SDK to export the private API, making it no-longer-private.

In other words, Wasp's private API is visible to the user.
Perhaps even worse, Wasp's private API is also visible to the IDE which suggest it thorough autoimports, further nudging the user to rely on undocumented or incomplete functionalities.

3 Possible approaches

I've identified two distinct approaches for making Wasp's private API truly private:

  1. Refactoring our codebase so that the SDK does not expose the private API (covered by #1841)
    In simpler terms: just move all the private stuff to the framework code and make sure the SDK doesn't need it.
    While obvious and seemingly the best solution, it does have its downsides - it's very time consuming, risks breaking something in Wasp, and would cause duplication whenever a public function shares implementation details with something from the framework code.
  2. Finding a way to hide the exposed private API from the user and their IDE
    If possible, doing so should give us an identical visible result with much less work and no duplication.

This PR proposes a way to implement the second approach.

4 The proposed solution

As a Proof of Concept, I've implemented all the proposed changes in a single "module" of our SDK (wasp/client/operations).

An SDK module exposes each symbol for one or more of the following reasons:

  1. The user needs it.
  2. Another module in the SDK needs it and uses a package import instead of a relative import.
  3. The framework's runtime needs it.

Important

We want to visibly expose only the first group: The imports our users need.

Fixing the second group of exposed imports (required by the SDK) is simple and only requires changing import paths.
Fixing the third group of exposed imports (required by the framework) is more complicated, but absolutely possible.

4.1 Code organization prerequisites

For the proposed approach to work, we must first do some reorganization:

  1. Ensure that the module's index file (e.g., wasp/client/operations/index.ts) exposes the public API and nothing else.
    Do this by removing all internal (SDK or framework) re-exports and relocate all direct exports.
    Done here.
  2. Accordingly update SDK import paths in the SDK code.
    Turn package imports into relative imports where necessary. For example, import x from 'wasp/client/operations' becomes import x form '../../client/operations'.
    This removes the need to expose imports needed in the SDK.
    Done here.
  3. Accordingly update SDK import paths in the framework code.
    Done here.

4.2 Hiding the private API

The above 3 steps remove the need to export private API used exclusively in the SDK and do some reorganization, but we're not done yet.

The main question has always been: How do we expose SDK's private API to the framework without exposing it to the user too?

This is where conditional exports come in:

  • Each node package can expose different paths and symbols depending on a condition string.
  • Each runtime can specify condition strings under which it operates.

The solution is to group the private API paths under a condition string (done here) that we only set when running our framework code (done here).

If we decide to implement this RFC, server and client will be setting different condition strings.

4.3 The only remaining problem

I don't want to be the "there's a bug in the compiler" guy again, but...

The setup I've described here works flawlessly... for packages that aren't local symlinks.

With our SDK (that is a local symlink), this happens:

initqueryclient.mp4

I can't get TypeScript's language server (TLS) to ignore the folder inside the .wasp directory.

Based on feedback I got from npx tsc --traceResolution, I'm guessing this happens because TLS knows about the package node_modules/wasp, but the folder being a symlink confuses TSL, which then starts treating the symlink's source folder as part of the user's code.

When I copy or hardlink the SDK into node modules, this doesn't happen. I'll report a bug to someone and see what they say.

@sodic sodic changed the title [RFC & POC] Hiding Private API [RFC & POC] Hiding Wasp's private API from users Mar 26, 2024
@sodic sodic changed the base branch from main to filip-autocomplete-imports March 26, 2024 15:48
@@ -30,6 +30,7 @@ const defaultViteConfig = {
outDir: "build",
},
resolve: {
conditions: ["client-runtime"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where we set the framework runtime's condition string, thus gaining access to the SDK's private API.

Comment on lines +12 to +14
"./client/operations/queryClient": {
"client-runtime": "./dist/client/operations/queryClient.js"
},
Copy link
Contributor Author

@sodic sodic Mar 27, 2024

Choose a reason for hiding this comment

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

This how we expose the SDK's private API to the framework, while hiding it from the user.

Unfortunately, we can't do:

"exports": {
  "client-runtime": {
    // private API
  },
  "default": {
    // public API
  }

Because once Vite finds an object with a matching condition, it won't trace back if it doesn't find a required symbol inside it:

Vite has a list of "allowed conditions" and will match the first condition that is in the allowed list.

Which means we would have to repeat the entire public API twice.

"exports": {
  "client-runtime": {
    // private API
    // public API
  },
  "default": {
    // public API
  }

It isn't quite as nice, but works. We can still decide which approach do we want to take.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great stuff! I didn't know that exports API worked this way. I always thought that the import and require were the only options.

Copy link
Contributor Author

@sodic sodic Mar 27, 2024

Choose a reason for hiding this comment

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

I've reorganized the index file to include export only the public API.

Comment on lines +3 to +4
import { useQuery } from '../client/operations'
import { addMetadataToQuery } from '../client/operations/queries/core'
Copy link
Contributor Author

@sodic sodic Mar 27, 2024

Choose a reason for hiding this comment

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

I've updated all the import paths inside the SDK have been converted from package imports to relative imports (also accounting for the updated paths).

@@ -7,7 +7,7 @@ import router from './router'
import {
initializeQueryClient,
queryClientInitialized,
} from 'wasp/client/operations'
} from 'wasp/client/operations/queryClient'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All SDK imports in the framework code have been updated according to the new code organization.

@sodic sodic marked this pull request as ready for review March 27, 2024 15:52
@Martinsos
Copy link
Member

Nice job @sodic , nice analysis!

It all sounds reasonable, and it is cool that there is such thing as conditional exports. How much is our solution "hacky" here -> is this what conditional exports are supposed to be used for?

What about that potential bug with symlink you mentioned -> did you get any response regarding that?

Additional idea, I will shortly explain it and you tell me if it doesn't make any sense: what if we had another package, called "wasp-core", that would contain logic from SDK that is used both by SDK and the framework code? And then that package would be used by SDK, but not reexported, and it would also be used by framework code. We would need to do refactoring, extract code from SDK into it, but result would achieve what we wanted, right? What would the pros/cons be here (if it works)?

Btw I will let @infomiho do the details review of this PR, while I will stick to high level stuff.

@sodic
Copy link
Contributor Author

sodic commented Mar 29, 2024

Is this hacky?

How much is our solution "hacky" here -> is this what conditional exports are supposed to be used for?

Not very hacky.

Conditional exports are meant to be used with user-provided strings and arbitrary semantics (read the docs for details).

Of course, I doubt anyone envisioned our case specifically (since we're so cutting edge :)), but it's not like we're abusing a completely unrelated mechanism.

How does it compare to the wasp-core idea?

What would the pros/cons be [of a hidden wasp-core package] (if it works)?

The most obvious pro is a very obvious separation between internal and external stuff.
Another pro is its simplicity - the mechanism it uses (two packages) is much simpler and more normal than conditional exports.

As for the cons...

This is another way of refactoring our code, so carries similar time implications. It also comes with additional cons that are a consequence of creating a different package.

I'll use this terminology when exploring the cons:

  • wasp-core - private SDK
  • wasp - public SDK

Anyway, the wasp-core approach:

  1. Takes a lot more time - self-explanatory.
  2. Complicates our setup - One more build step, another package.json and tsconfig.json to maintain, one more folder added to the Docker's build context...
  3. Creates an extra layer of separation - More precisely, our SDK is no longer organized by "features" (i.e., all auth stuff goes here). The first layer of separation now depends on whether something is internal (wasp-core) or external (wasp), which is not as nice. It creates yet another file tree of similarly named files and folders, which makes it easier to edit the wrong stuff during development
  4. Makes future refactors and changes difficult - With the conditional exports approach, refactors and visibility changes are pretty straightforward. With the wasp-core approach, not so much. I'll give two examples:
    • Example 1: Imagine a very local function used only in wasp (it sits in wasp/something and is not exported). Now imagine we want to reuse this function in our framework code. We have to move it to wasp-core and change all the imports in wasp.
    • Example 2: Imagine we want to make a private function in wasp-core public. Let's assume the function is used by both wasp and the framework code. To make it public, we must: change the function's location (move from wasp-core to wasp), update all the imports in the SDK, update all the imports in the framework code, and possibly do some refactoring (if other functions in the wasp-core depended on it)
  5. Doesn't hide anything from the user - They can still import and get auto-suggestions from wasp-core. Sure, we could add a compile-time check that prohibits any wasp-core imports, but that's another complication. Or we could use conditional exports for the wasp-core stuff, but then we could have just used that in the first place.

Alternative to the alternative

@infomiho suggested that, instead of the currently proposed separation between wasp-core and wasp, we have:

  • wasp-core - All SDK functionality and implementations (both public and private).
  • wasp - Exclusively reexports stuff from wasp-core we want to make public (no implementations).

This approach is slightly better because it solves problem 4 (difficult to change visibility and refactor), but still suffers from the rest of the issues.

What about the symlink?

What about that potential bug with symlink you mentioned -> did you get any response regarding that?

I'll update the PR's description with the issue link once as soon as I report it. Just to be clear: this isn't related to conditional exports in any way, I've tested it with and without them.

Still, even if we doesn't get fixed immediately, implementing conditional exports would still be an improvement - that long import looks very out of place, and users will have an easier time identifying a mistake.

Base automatically changed from filip-autocomplete-imports to main March 29, 2024 15:10
@Martinsos
Copy link
Member

Is this hacky?

How much is our solution "hacky" here -> is this what conditional exports are supposed to be used for?

Not very hacky.

Conditional exports are meant to be used with user-provided strings and arbitrary semantics (read the docs for details).

Of course, I doubt anyone envisioned our case specifically (since we're so cutting edge :)), but it's not like we're abusing a completely unrelated mechanism.

How does it compare to the wasp-core idea?

What would the pros/cons be [of a hidden wasp-core package] (if it works)?

The most obvious pro is a very obvious separation between internal and external stuff. Another pro is its simplicity - the mechanism it uses (two packages) is much simpler and more normal than conditional exports.

As for the cons...

This is another way of refactoring our code, so carries similar time implications. It also comes with additional cons that are a consequence of creating a different package.

I'll use this terminology when exploring the cons:

  • wasp-core - private SDK
  • wasp - public SDK

Anyway, the wasp-core approach:

  1. Takes a lot more time - self-explanatory.

  2. Complicates our setup - One more build step, another package.json and tsconfig.json to maintain, one more folder added to the Docker's build context...

  3. Creates an extra layer of separation - More precisely, our SDK is no longer organized by "features" (i.e., all auth stuff goes here). The first layer of separation now depends on whether something is internal (wasp-core) or external (wasp), which is not as nice. It creates yet another file tree of similarly named files and folders, which makes it easier to edit the wrong stuff during development

  4. Makes future refactors and changes difficult - With the conditional exports approach, refactors and visibility changes are pretty straightforward. With the wasp-core approach, not so much. I'll give two examples:

    • Example 1: Imagine a very local function used only in wasp (it sits in wasp/something and is not exported). Now imagine we want to reuse this function in our framework code. We have to move it to wasp-core and change all the imports in wasp.
    • Example 2: Imagine we want to make a private function in wasp-core public. Let's assume the function is used by both wasp and the framework code. To make it public, we must: change the function's location (move from wasp-core to wasp), update all the imports in the SDK, update all the imports in the framework code, and possibly do some refactoring (if other functions in the wasp-core depended on it)
  5. Doesn't hide anything from the user - They can still import and get auto-suggestions from wasp-core. Sure, we could add a compile-time check that prohibits any wasp-core imports, but that's another complication. Or we could use conditional exports for the wasp-core stuff, but then we could have just used that in the first place.

Alternative to the alternative

@infomiho suggested that, instead of the currently proposed separation between wasp-core and wasp, we have:

  • wasp-core - All SDK functionality and implementations (both public and private).
  • wasp - Exclusively reexports stuff from wasp-core we want to make public (no implementations).

This approach is slightly better because it solves problem 4 (difficult to change visibility and refactor), but still suffers from the rest of the issues.

What about the symlink?

What about that potential bug with symlink you mentioned -> did you get any response regarding that?

I'll update the PR's description with the issue link once as soon as I report it. Just to be clear: this isn't related to conditional exports in any way, I've tested it with and without them.

Still, even if we doesn't get fixed immediately, implementing conditional exports would still be an improvement - that long import looks very out of place, and users will have an easier time identifying a mistake.

Ok! Convincing analysis, makes sense to me. I am ok with sticking with the approach you started in this PR, and it is nice to know we potentially also have some other options in the future also on the table.

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