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

[rush] Design Idea: rush-sdk should be loaded explicitly #3209

Open
octogonz opened this issue Feb 5, 2022 · 3 comments
Open

[rush] Design Idea: rush-sdk should be loaded explicitly #3209

octogonz opened this issue Feb 5, 2022 · 3 comments

Comments

@octogonz
Copy link
Collaborator

octogonz commented Feb 5, 2022

Summary

Under the current design, if a standalone tool calls require('@rushstack/rush-sdk') this may..

  • trigger install-run-rush.js to automatically install NPM packages
  • print console output
  • possibly throw an exception if an error occurs

This magic has a few downsides:

  • The above operations happen implicitly on the first call to require('@rushstack/rush-sdk'), nothing about the API signature indicates that this particular call is different from a conventional import
  • Adding a try/catch block is slightly awkward
  • We can't pass options to require('@rushstack/rush-sdk')

Improved Approach 1

Suppose that we change the design so that a special startup call is required to load rush-sdk:

// By itself, this throws an exception because `rush-sdk` has not been initialized yet:
const rushSdk = require('rush-sdk')

The special startup call should get executed at the very beginning of your tool/script before other dependencies are imported:

const { RushSdkLoader } = require('rush-sdk/loader');

try {
  // Prints to the console
  RushSdkLoader.install();

  // But we could optionally print to an ITerminal instead:
  // RushSdkLoader.install({ terminal: getMyTerminal() });
} catch (e) {
  // And the caller can choose how to report errors:
  console.log(e.message);
}

// After the special startup code has done its thing, require() now succeeds:
const rushSdk = require('rush-sdk');

import { YourStuff } from 'rest-of-your-app';

Improved Approach 2

Rather than having a special entry point require('rush-sdk/loader'), maybe we could fold this definition into the proxy somehow?

// In other words, we allow this to succeed...
const rushSdk = require('rush-sdk');

// ...whereas Property access would throw an exception because the SDK has not been loaded yet:
//const config = rushSdk.RushConfiguration().loadFromDefaultLocation();

const { RushSdkLoader } = require('rush-sdk');
RushSdkLoader.install();

// This succeeds
const config = rushSdk.RushConfiguration().loadFromDefaultLocation();

This is more concise, but slightly more magical. Maybe Webpack would have trouble bundling it?

CC @iclanton @elliot-nelson @chengcyber

@elliot-nelson
Copy link
Collaborator

Of the two, I think I prefer (1), because it changes the api the least -- require still does the loading and confirmation.

Another option in my opinion would be to add the { RushSdkLoader } interface where you can control when and how the api is loaded, but if you don't use it, continue to support the "magic" version already implemented. This works both for one-off user land scripts and for larger scripts that need to exert more control.

@iclanton
Copy link
Member

iclanton commented Feb 6, 2022

I agree with @elliot-nelson. I put that together here: #3212

@chengcyber
Copy link
Contributor

+1 for Approach 1.

For now, call import {} from 'rush-sdk' is same as import {} from 'rush-lib'. It's good to keep this mental model: rush-sdk equals rush-lib somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: General Discussions
Development

No branches or pull requests

4 participants