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

fix typescript: translate options.sourceMap to options.compilerOptions.sourceMap (#286) #299

Merged
merged 10 commits into from
Jan 21, 2021

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Jan 18, 2021

trivial : )

edit: less trivial on second sight
in the old version, sveltePreprocess({ sourceMap: true })
was translated to transformer({ content, filename, options: { sourceMap: true } })
but the typescript transformer function expects compiler-options in options.compilerOptions
so the correct translation is transformer({ content, filename, options: { compilerOptions: { sourceMap: true } } })

@milahu milahu mentioned this pull request Jan 18, 2021
@dummdidumm
Copy link
Member

Not sure if this is the correct fix, since other languages do not output sourcemaps by default. I think instead it would be better if the sourceMap option that you can pass into svelte-preprocess is used.

line ~105

const compilerOptions: CompilerOptions = {
    ...(convertedCompilerOptions as CompilerOptions),
    importsNotUsedAsValues: ts.ImportsNotUsedAsValues.Error,
    allowNonTsExtensions: true,
    sourceMap: !!options.sourceMap // <<-- new
  };

@milahu
Copy link
Contributor Author

milahu commented Jan 20, 2021

thanks, fixed

edit: typescript says no

TSError: src/transformers/typescript.ts:108:26 - error TS2339: Property 'sourceMap' does not exist on type 'Typescript'.

but ...

// typescript/lib/typescript.d.ts
    export interface CompilerOptions {
// ...
        sourceMap?: boolean;

@dummdidumm
Copy link
Member

dummdidumm commented Jan 20, 2021

My bad, I thought options is what one can pass in into sveltePreprocess({..}). So I guess some more digging is needed on how to get that option in there (Disclaimer: I know the codebase just as much/little as you 😄 ).

@milahu
Copy link
Contributor Author

milahu commented Jan 20, 2021

looks like a bug in getTransformerOptions

    if (sourceMap && name in SOURCE_MAP_PROP_MAP) {
      const [propName, value] = SOURCE_MAP_PROP_MAP[name];

      opts[propName] = value;
    }
export const SOURCE_MAP_PROP_MAP: Record<string, [string, any]> = {
  babel: ['sourceMaps', true],
  typescript: ['sourceMap', true],

maybe the key should be compilerOptions.sourceMap
but then opts[propName] is broken, of course

long story
// svelte-preprocess/src/transformers/typescript.ts
const transformer: Transformer<Options.Typescript> = ({
  content,
  filename,
  options = {},
}) => {
// svelte-preprocess/src/autoProcess.ts
export const transform = async (
  name: string,
  options: TransformerOptions,
  { content, map, filename, attributes }: TransformerArgs<any>,
): Promise<Processed> => {
  if (options === false) {
    return { code: content };
  }

  if (typeof options === 'function') {
    return options({ content, map, filename, attributes });
  }

  // todo: maybe add a try-catch here looking for module-not-found errors
  const { transformer } = await import(`./transformers/${name}`);

  return transformer({
    content,
    filename,
    map,
    attributes,
    options: typeof options === 'boolean' ? null : options,
  });
// svelte-preprocess/src/autoProcess.ts
    const transformerOptions = getTransformerOptions(lang, alias);
// ...
    const transformed = await transform(lang, transformerOptions, {
      content,
      filename,
      attributes,
    });
// svelte-preprocess/src/autoProcess.ts
  const getTransformerOptions = (
    name: string,
    alias?: string,
  ): TransformerOptions<unknown> => {
    const { [name]: nameOpts, [alias]: aliasOpts } = transformers;

    if (typeof aliasOpts === 'function') return aliasOpts;
    if (typeof nameOpts === 'function') return nameOpts;
    if (aliasOpts === false || nameOpts === false) return false;

    const opts: Record<string, any> = {};

    if (typeof nameOpts === 'object') {
      Object.assign(opts, nameOpts);
    }

    Object.assign(opts, getLanguageDefaults(name), getLanguageDefaults(alias));

    if (name !== alias && typeof aliasOpts === 'object') {
      Object.assign(opts, aliasOpts);
    }

    if (sourceMap && name in SOURCE_MAP_PROP_MAP) {
      const [propName, value] = SOURCE_MAP_PROP_MAP[name];

      opts[propName] = value;
    }

    return opts;
  };
// svelte-preprocess/src/autoProcess.ts
export function sveltePreprocess(
  {
    aliases,
    markupTagName = 'template',
    preserve = [],
    defaults,
    sourceMap = process?.env?.NODE_ENV === 'development' ?? false,
    ...rest
  } = {} as AutoPreprocessOptions,
): AutoPreprocessGroup {
  const defaultLanguages = Object.freeze({
    markup: 'html',
    style: 'css',
    script: 'javascript',
    ...defaults,
  });

  const transformers = rest as Transformers;

@dummdidumm
Copy link
Member

This will also fix this issue

@milahu milahu changed the title fix typescript: make sourcemaps default on (#286) fix typescript: translate options.sourceMap to options.compilerOptions.sourceMap (#286) Jan 20, 2021
@kaisermann
Copy link
Member

kaisermann commented Jan 20, 2021

Hey, @milahu 👋 thanks for the contribution! I'm not sure if I'm a big fan of duplicating almost the same code. I think we could extract it to a function or use lukeed's dset (which is very tiny, as usual. current solution weights roughly 291b vs 160b). What do you think? cc @dummdidumm

@milahu
Copy link
Contributor Author

milahu commented Jan 21, 2021

hey dawg, i heard you like small functions, so me and my man terser, we made you a 124 byte function

function setProp(t,...o){let e=0;
for(;e<o.length-2;e++){const p=o[e];"object"!=typeof t[p]&&(t[p]={}),t=t[p]}
t[o[e]]=o[e+1]}
verbose
// args = array of prop-names, last arg = value
function setProp(obj, ...args) {
  let i = 0;

  for (; i < args.length - 2; i++) {
    const key = args[i];

    if (typeof obj[key] !== 'object') {
      obj[key] = {};
    }

    obj = obj[key];
  }

  //const key = args[i];
  //const val = args[i + 1];
  //obj[key] = val;
  
  obj[args[i]] = args[i + 1];
}

const SOURCE_MAP_PROP_MAP = {
  typescript: ['compilerOptions', 'sourceMap', true],
};
const opts = {};
const name = 'typescript';
setProp(opts, ...SOURCE_MAP_PROP_MAP[name]);
console.dir(opts);
// --> { compilerOptions: { sourceMap: true } }

move to svelte-preprocess/src/modules/utils.ts?

@kaisermann
Copy link
Member

😆 If we're going to maintain it, I tend to prefer the verbose version heh. utils.ts is fine!

@kaisermann kaisermann self-assigned this Jan 21, 2021
src/modules/utils.ts Outdated Show resolved Hide resolved
src/autoProcess.ts Show resolved Hide resolved
Copy link
Member

@kaisermann kaisermann left a comment

Choose a reason for hiding this comment

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

accidentally approved it, heh sorry

@kaisermann kaisermann merged commit c8a3cd6 into sveltejs:main Jan 21, 2021
@kaisermann
Copy link
Member

kaisermann commented Jan 21, 2021

Thanks! Should be up in v4.6.2 🎉

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