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 runtime compiler APIs #4752

Closed
kitsonk opened this issue Apr 14, 2020 · 14 comments · Fixed by #8799
Closed

Refactor runtime compiler APIs #4752

kitsonk opened this issue Apr 14, 2020 · 14 comments · Fixed by #8799
Assignees
Labels
breaking change a change or feature that breaks existing semantics cli related to cli/ dir public API related to "Deno" namespace in JS

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Apr 14, 2020

We recently changed deno fetch to deno cache. When we introduced the runtime compiler APIs, one of the challenges is that it didn't align well with the subcommands. Given that we have a better name now for the subcommands, it feels right to re-align the runtime compiler APIs, also based on some experience with them as well. I am recommending the following:

namespace Deno {
  export interface CacheOptions { /* would be close to existing compiler options */ }
  export interface BundleOptions ( /* would vary a little bit from cache options */ }
  export interface TranspileOptions extends CacheOptions { /* a few options differ */ }

  export function cache(root: string, options?: RuntimeCompilerOptions): Promise<Record<string, string>>;
  export function bundle(root: string, options?: RuntimeCompilerOptions): Promise<string>;
  export function transpile(sources: Record<string, string>, options?: TranspileOptions): Promise<Record<string, string>>;
}

Deno.cache would work like deno cache and the RuntimeCompilerOptions would also support the flags that deno cache supports plus the support of an expanded tsconfig.json set of compiler options aligned to what exists today in CompilerOptions.

Deno.bundle would produce a single file emit, like deno bundle does and people would use Deno.transpile() when they have sources that they want the compiler to transpile for them.

We could introduce the equivalent --no-check in these APIs as well, as well as adding it to the appropriate subcommands, but that might be an enhancement on top of this refactor.

Refs: #4600

@jakajancar
Copy link

Looks good.

Right now --cached-only on the CLI affects Deno.compile(), which is weird. Would this still be the case? I would expect the CLI settings/tsconfig.json would affect how deno runs my app, not the logic of my code itself (action at a distance-ish). E.g. I might want --cached-only to ensure my app has all files downloaded, buy maybe it's a watching/reloading compiler for bundling web stuff, so obviously it will not use the cache only when building it's output.

@Soremwar
Copy link
Contributor

Soremwar commented May 7, 2020

Should this be done before v1.0? If not, then compile should probably be hidden under the --unstable flag in order to be changed in the future.

@kitsonk
Copy link
Contributor Author

kitsonk commented May 7, 2020

Isn't it under --unstable?

@Soremwar
Copy link
Contributor

Soremwar commented May 8, 2020

It is 😅

Sorry for my blindness

@Nautigsam
Copy link
Contributor

Nautigsam commented May 18, 2020

I am trying to use Deno.bundle for a svelte application. I am having a hard time resolving imports and maybe a change in the API could help.
To illustrate this I show you the compiled version of the svelte components (only the imports/exports matter here):

// build/js/Text.js
import {
  ...
} from "https://dev.jspm.io/svelte/internal";
...
export default Component;

// build/js/App.js
import {
  ...
} from "https://dev.jspm.io/svelte/internal";
import Text from "./Text.svelte";
...
export default Component;

You can see the svelte compiler does not change the import of Text.svelte component. I guess we can not change it since each component is compiled individually.
I think I could give a resolver function to Deno.bundle which would be called for every import so I could either let Deno resolve by returning immediately or return a custom module/path/whatever if I know Deno will not be able to resolve the import.
I don't know how it is related to import-maps, which I don't know well.

@jimsimon
Copy link

jimsimon commented May 18, 2020

Coming from #5562...

I would love to see BundleOptions have a property (maybe of type CodeSplitOptions?) to configure the behavior of code splitting. Ideally it could be disabled entirely or enabled with full vendor and dyanmic import chunking support. I'd start simple by covering the 80% use case (essentially webpack's default behavior) and entertain more options later. The initial implementation could be a simple boolean enableCodeSplitting property, but I think having an object for the config allows for future customization without having to change the API again. Alternatively there could be both an enableCodeSplitting boolean property and then a config separate codeSplittingConfig property. I'm not married to any specific approach, but I do think starting with something as simple as possible and expanding from there will limit the scope of these changes.

Taking a step back, I'd personally love to see a more granular API. It seems like this ticket is heading that direction a little bit. While the CLI is going to be more porcelain, I think the API can offer up commands for the various compilation and execution stages. To be fair I haven't looked too deeply into the compiler code, but an api with the following functions would offer maximum flexibility for custom tooling:

namespace Deno {
  export interface FetchOptions { /* fetch specific options */ }
  export interface CacheOptions { /* would be close to existing compiler options */ }
  export interface BundleOptions ( /* would vary a little bit from cache options */ }
  export interface TranspileOptions { /* a few options differ */ }
  export interface WriteOptions { /* likely fairly simple -- maybe options for output directory, permissions, etc */ }

  export function fetch(root: string, options?: FetchOptions): Promise<Record<string, string>>;
  export function cache(sources: Record<string, string>, options?: CacheOptions): Promise<Record<string, string>>;
  export function transpile(sources: Record<string, string>, options?: TranspileOptions): Promise<Record<string, string>>;
  export function bundle(sources: Record<string, string>, options?: BundleOptions): Promise<Record<string, string>>; // Note: return type changed to support code splitting
  export function write(sources: Record<string, string>, options?: WriteOptions): Future<Array<String>>; // Returns the filepaths of the written files
}

If you wanted to completely fetch, cache, transpile, and bundle you'd do something like...

async function main() {
  const fetched = await fetch('./my-entrypoint.ts', fetchOptions)
  const cached = await cache(fetched, cacheOptions)
  const transpiled = await transpile(cached, transpileOptions)
  const bundled = await bundle(transpiled, bundleOptions)
  const writtenFilepaths = await write(bundled, writeOptions)

  // Added for illustrative purposes
  const p = Deno.run({
    cmd: ["deno", "run", writtenFilepaths[0]], // Assumes a single output file or that the first file is runnable
  });
  await p.status()
}

I'd also personally love to see TranspileOptions include the ability to specify typescript transformers (#3354), but I'll leave discussion about the merits of them to that other issue.

One last thing that might be neat would be a run function, but that's technically probably outside of the scope of a compiler API. I'm not sure if this is being viewed as strictly a compiler api or maybe something a little more generic like a "Deno Programmatic API". If a run function existed, I'd expect it would look something like:

  // RunOptions is the same interface that Process uses via the current Deno.run() api call.
  export interface DenoRunOptions extends RunOptions { /* allow setting read, write, net, unstable, etc permissions */ }

  // This name will conflict with the current Deno.run() function but I can't think of a better name at the moment.
  // Maybe this entire api should be under its own namespace?
  export function run(filepath: string, options?: DenoRunOptions): Future<Process>; // returns an instance of Deno.Process

@bartlomieju
Copy link
Member

bartlomieju commented May 20, 2020

My 2 cents:

  1. I really don't like cache name - I think it's confusing - in case of passing map of sources to this function, the compiled source are never cached but returned as emitMap. Caching is just a side effect of this function under specific conditions, it's main purpose is to type check and transpile TS to JS.
  2. Return type of each function should be an interface so it is extensible in the future:
export function cache(root: string, options?: RuntimeCompilerOptions): Promise<{
   emitMap: Record<string, string>,
   diagnostics: Diagnostic[],
}>;
export function bundle(root: string, options?: RuntimeCompilerOptions): Promise<{
  bundle: string,
  diagnostics: Diagnostic[],
}>;
export function transpile(sources: Record<string, string>, options?: TranspileOptions): Promise<{
   emitMap: Record<string, string>,
   diagnostics: Diagnostic[],
}>;

@jimsimon
Copy link

Under my proposed API, the point of the cache function is just to cache the files. The emitMap is just returned as a convenience and should be identical to what the fetch function returns. The cache function could also just return a Promise that resolves on completion of returning an emitMap seems confusing.

The goal of breaking the API apart like I did is to allow tools to make changes to files at each step of the process. Callback functions could accomplish the same result if a broken apart API isn't desirable.

@bartlomieju bartlomieju added cli related to cli/ dir public API related to "Deno" namespace in JS labels May 21, 2020
@sethp
Copy link

sethp commented Jul 15, 2020

I'm curious about the separation between runtime and, I guess, ahead-of-time compile APIs – is the separation intentional, or more mechanical?

In other words, I'm wondering what you'd think of a world where deno bundle invokes some "built-in" typescript snippet that calls Deno.bundle(), if such a mechanism existed for sub-commands?

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 15, 2020

Mostly because there are subtle differences, partly because of how they came about. The "internal" compiler API was first, which was really focused on hydrating the cache with compiled code. Then along came the bundle one, and it needed to produce a return value as well as hydrating the cache. The run time APIs came along later, and they had a slightly different set of needs.

The refactor suggested here would likely align the internal and runtime APIs a bit better and "might" make it so we have less code paths through the compiler (there are 5 "major" ones at the moment, with some significant branches in each). @bartlomieju @ry and myself have done a lot of iteration on it over the past couple of years and it isn't pretty. In addition to that, we have been working on #5432 which is moving stuff around quite a lot, making the whole thing a bit more ugly.

That being said it would never call Deno.bundle() directly, but deno bundle and Deno.bundle() should in theory call the same internal Rust API (well specifically Deno.bundle() would call a "op" which in turn would call the same Rust API that deno bundle sub-command does).

@nayeemrmn
Copy link
Collaborator

Do Deno.cache() and Deno.bundle() support both usages of 1) passing the name of an external root with caching in DENO_DIR and 2) passing a map of sources and processing without side-effects? That's what #4752 (comment) seems to suggest. I'm wondering how this should affect Deno.ast(): #6893 (comment).

@bartlomieju bartlomieju self-assigned this Sep 15, 2020
@bartlomieju bartlomieju mentioned this issue Oct 10, 2020
22 tasks
@kitsonk kitsonk added the breaking change a change or feature that breaks existing semantics label Nov 2, 2020
@kitsonk kitsonk changed the title [BREAKING CHANGE] Refactor runtime compiler APIs Refactor runtime compiler APIs Nov 2, 2020
@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 2, 2020

Been doing a lot of thinking about this recently. I believe this is the interface we should adopt:

declare namespace Deno {
  export interface CompilerOptions {
    /* effectively what we currently have in unstable */
  }

  export interface EmitOptions {
    /** Indicate that the source code should be emitted to a single file
     * JavaScript bundle that is either a ES module (`esm`) or a immediately
     * invoked function execution (`iife`). */
    bundle?: "esm" | "iife";
    /** If `true` then the sources will be typed checked, returning any
     * diagnostic errors in the result.  If `false` type checking will be
     * skipped.  Defaults to `true`.
     * 
     * *Note* by default, only TypeScript will be type checked, just like on
     * the command line.  Use the `compilerOptions` options of `checkJs` to
     * enable type checking of JavaScript. */
    check?: boolean;
    /** A set of options that are aligned to TypeScript compiler options that
     * are supported by Deno. */
    compilerOptions?: CompilerOptions;
    /** A record of sources to use when doing the emit.  If provided, Deno will
     * use these sources
     */
    sources?: Record<string, string>;
  }

  export interface EmitResult {
    /** Diagnostic messages returned from the type checker (`tsc`). */
    diagnostics: Diagnostic[];
    /** Any emitted files.  If bundled, then the JavaScript will have the
     * key of `deno:///bundle.js` with an optional map (based on
     * `compilerOptions`) in `deno:///bundle.js.map`. */
    files: Record<string, string>;
    /** An array of internal statics related to the emit, for diagnostic
     * purposes. */
    stats: Array<[string, number]>;
  }

  /**
   * 
   * @param rootName The specifier that will be used as the entry point.  If no
   *                 sources are provided, then the specifier would be the same
   *                 as if you typed it on the command line for `deno run`.  If
   *                 sources are provided, it should match one of the names of
   *                 the sources.
   * @param options  A set of options to be used with the emit.
   */
  export function emit(
    rootSpecifier: string,
    options?: EmitOptions,
  ): Promise<EmitResult>;
}

This would provide all the existing functionality (plus more) in a single API that would serve as a general purpose source code emitter and type checker.

@bartlomieju
Copy link
Member

@kitsonk I like this API, it nicely corresponds to current Rust API

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 5, 2020

Thought about this more, forgot about cache busting:

export interface EmitOptions {
  /** Indicate that the source code should be emitted to a single file
   * JavaScript bundle that is either a ES module (`esm`) or a immediately
   * invoked function execution (`iife`). */
  bundle?: "esm" | "iife";
  /** This impacts the behaviour of the internal Deno sources cache.  `"use"`
   * indicates that the cache should be used, meaning remote modules already
   * in the cache will not be fetched. `"reload"` indicates that remote sources
   * should be requested again and updated in the cache if changed, and `"only"`
   * indicates that only remote sources that already exist in the cache can be
   * used. If a source is not in the cache, it will result in a not found error
   * being thrown on the request.
   * 
   * This default is `"use"`. */
  cache?: "use" | "reload" | "only";
  /** If `true` then the sources will be typed checked, returning any
   * diagnostic errors in the result.  If `false` type checking will be
   * skipped.  Defaults to `true`.
   * 
   * *Note* by default, only TypeScript will be type checked, just like on
   * the command line.  Use the `compilerOptions` options of `checkJs` to
   * enable type checking of JavaScript. */
  check?: boolean;
  /** A set of options that are aligned to TypeScript compiler options that
   * are supported by Deno. */
  compilerOptions?: CompilerOptions;
  /** A record of sources to use when doing the emit.  If provided, Deno will
   * use these sources
   */
  sources?: Record<string, string>;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change a change or feature that breaks existing semantics cli related to cli/ dir public API related to "Deno" namespace in JS
Projects
None yet
8 participants