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

Implement NormalizedInputOptions and NormalizedOutputOptions to rust side #1041

Open
hyf0 opened this issue May 6, 2024 · 4 comments
Open

Comments

@hyf0
Copy link
Member

hyf0 commented May 6, 2024

Why

The main problem is that the concept NormalizedInputOptions is try to give a final form of options while it still in Js side not passing to the rust.

Take external option as an example, NormalizedInputOptions require external option to be a function, which means though users passed static ['external'] data and it would still needs be wrapped in a js function, that's bad for performance. If external option is setted, every resolve process would need to call Js function in sequence.

How

I looked into the ``NormalizedInputOptions` and delete we highly not going to support. We got

export interface NormalizedInputOptions {
  // acorn: Record<string, unknown>
  // acornInjectPlugins: (() => unknown)[]
  // cache: false | undefined | RollupCache
  context: string
  experimentalCacheExpiry: number
  experimentalLogSideEffects: boolean
  external: IsExternal
  /** @deprecated Use the "inlineDynamicImports" output option instead. */
  inlineDynamicImports: boolean | undefined
  input: string[] | { [entryAlias: string]: string }
  logLevel: LogLevelOption
  makeAbsoluteExternalsRelative: boolean | 'ifRelativeSource'
  /** @deprecated Use the "manualChunks" output option instead. */
  manualChunks: ManualChunksOption | undefined
  // maxParallelFileOps: number
  /** @deprecated Use the "maxParallelFileOps" option instead. */
  // maxParallelFileReads: number
  moduleContext: (id: string) => string
  onLog: LogHandler
  onwarn: (warning: RollupLog) => void
  perf: boolean
  plugins: Plugin[]
  preserveEntrySignatures: PreserveEntrySignaturesOption
  /** @deprecated Use the "preserveModules" output option instead. */
  preserveModules: boolean | undefined
  preserveSymlinks: boolean
  shimMissingExports: boolean
  strictDeprecations: boolean
  treeshake: false | NormalizedTreeshakingOptions
}

The problem here is that for properties whose value is function is complicated/impossible to pass them from rust to js, so we only have

export interface NormalizedInputOptions {
  context: string
  experimentalCacheExpiry: number
  experimentalLogSideEffects: boolean
  /** @deprecated Use the "inlineDynamicImports" output option instead. */
  inlineDynamicImports: boolean | undefined
  input: string[] | { [entryAlias: string]: string }
  logLevel: LogLevelOption
  makeAbsoluteExternalsRelative: boolean | 'ifRelativeSource'
  perf: boolean
  preserveEntrySignatures: PreserveEntrySignaturesOption
  /** @deprecated Use the "preserveModules" output option instead. */
  preserveModules: boolean | undefined
  preserveSymlinks: boolean
  shimMissingExports: boolean
  strictDeprecations: boolean
  treeshake: false | NormalizedTreeshakingOptions
}

I kind of feel this is enough for compatibility support. I hardly think of the situation the users want to "have" the function in the NormalizedInputOptions and do something with it. Unless they want to do something hacky, but that's not our scope.

The above analysis also applies on NormalizedOutputOptions. As long as we don't consider properties whose value is function, it's all fine.

As a bonus:

  • Parallel js plugins could also visit options even in worker Enviromint.
@underfin
Copy link
Contributor

underfin commented May 7, 2024

Take external option as an example, NormalizedInputOptions require external option to be a function, which means though users passed static ['external'] data and it would still needs be wrapped in a js function, that's bad for performance. If external option is setted, every resolve process would need to call Js function in sequence.

I also think about it before, we could using original external option to improve it. If the user not changed normalized external option, we just using original external option to binding option, it could be array|function.

I‘m worried if we remove the function value fields at NormalizedInputOptions, it maybe caused some problems at plugin compat.

@hyf0
Copy link
Member Author

hyf0 commented May 7, 2024

I also think about it before, we could using original external option to improve it. If the user not changed normalized external option, we just using original external option to binding option, it could be array|function.

I think about this too. But it requires us to change the definition of NormalizedInputOptions. If this is acceptable, we could just use InputOptions as the NormalizedInputOptions. I kind of feel that this makes the normalization step in js side meaningless, so I didn't propose this way.

I‘m worried if we remove the function value fields at NormalizedInputOptions, it maybe caused some problems at plugin compat.

About this I said above

I hardly think of the situation the users want to "have" the function in the NormalizedInputOptions and do something with it. Unless they want to do something hacky, but that's not our scope.

In fact, the current NormalizedInputOptions is already freeze, so NormalizedInputOptions is read-only. For a read-only NormalizedInputOptions, access the external option seems meaningless.

@underfin
Copy link
Contributor

Here find a usage at astro, it need to mutate plugins. see here.

Here has a more hack usage.. see here

@hyf0
Copy link
Member Author

hyf0 commented May 29, 2024

Here find a usage at astro, it need to mutate plugins. see here.

options hooks stays in Js side., so this is fine.

Here has a more hack usage.. see here

Guess it would be ok to switch to options hook. I don't see anything special that make it have to use NormalizedOptions in buildStart hook.

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

No branches or pull requests

2 participants