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

add enable_import_extensions option #233

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

Conversation

nvandamme
Copy link

No description provided.

@timostamm
Copy link
Owner

Thanks for this, Nicolas!

I think that CI fails because the old TypeScript version being used doesn't understand .js extensions in import statements.

If you were to bump TypeScript in packages/test-generated/package.json to a recent version, I think test-generated should pass, which will be a good proof that it works. Would you like to do so?

CI failure log:

@protobuf-ts/test-generated: Error: Cannot find module './google/protobuf/empty.js'
@protobuf-ts/test-generated: Require stack:
@protobuf-ts/test-generated: - /home/circleci/project/packages/test-generated/ts-out/exclude-options.ts
@protobuf-ts/test-generated: - /home/circleci/project/packages/test-generated/spec/exclude-options.spec.ts
@protobuf-ts/test-generated: - /home/circleci/project/packages/test-generated/node_modules/jasmine/lib/jasmine.js
@protobuf-ts/test-generated: - /home/circleci/project/packages/test-generated/node_modules/jasmine/bin/jasmine.js
@protobuf-ts/test-generated:     at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1030:15)
@protobuf-ts/test-generated:     at Function.Module._resolveFilename (/home/circleci/project/packages/test-generated/node_modules/tsconfig-paths/lib/register.js:75:40)
@protobuf-ts/test-generated:     at Function.Module._load (internal/modules/cjs/loader.js:899:27)
@protobuf-ts/test-generated:     at Module.require (internal/modules/cjs/loader.js:1090:19)
@protobuf-ts/test-generated:     at require (internal/modules/cjs/helpers.js:75:18)
@protobuf-ts/test-generated:     at Object.<anonymous> (/home/circleci/project/packages/test-generated/ts-out/exclude-options.ts:4:1)
@protobuf-ts/test-generated:     at Module._compile (internal/modules/cjs/loader.js:1201:30)
@protobuf-ts/test-generated:     at Module.m._compile (/home/circleci/project/packages/test-generated/node_modules/ts-node/src/index.ts:858:23)
@protobuf-ts/test-generated:     at Module._extensions..js (internal/modules/cjs/loader.js:1221:10)
@protobuf-ts/test-generated:     at Object.require.extensions.<computed> [as .ts] (/home/circleci/project/packages/test-generated/node_modules/ts-node/src/index.ts:861:12)
@protobuf-ts/test-generated: Makefile:45: recipe for target 'test-spec' failed

@nvandamme
Copy link
Author

nvandamme commented Feb 28, 2022

Hi @timostamm ,

Thanks for the feedback, sure!

For now, i'm using a quick'n dirty script on my side to ensure all local imports has an extension.
This script is ran after protobuf-ts generation on all *.ts and *.js generated files :

import * as fs from "fs";
import glob from "glob";

function add_import_extensions(er, files) {
    files.forEach(file => {
        let data = fs.readFileSync(file, {encoding: "utf8"});
        data = data.replace(/from\ "\.\/(((?!\.js).)*)";$/gm, "from \"./$1.js\";");
        data = data.replace(/from\ "\.\.\/(((?!\.js).)*)";$/gm, "from \"../$1.js\";");
        fs.writeFileSync(file, data);
    });
}

glob("./api/js/**/*.ts", {}, add_import_extensions);
glob("./api/js/**/*.js", {}, add_import_extensions);

@debkanchan
Copy link

@nvandamme any updates on this? Since ts 4.7 released this is crucial

@faustbrian
Copy link

Any update on this? ESM is becoming more widespread so would be nice to have this added.

@QuantumGhost QuantumGhost mentioned this pull request Aug 22, 2022
@sachaw
Copy link

sachaw commented Sep 19, 2022

@timostamm are you happy with this implementation, or are there changes you want performed, I've just hit a case where this is now a blocker, thanks.

@smnbbrv
Copy link

smnbbrv commented Oct 24, 2022

for those who suffers and cannot wait until this gets merged, just run this after generation

// proto-patch.js
import glob from "glob";
import fs from "fs";

const protoRoot = 'srv/proto';

glob(protoRoot + '/**/*.ts', async (err, files) => {
  files.forEach(file => {
    let content = fs.readFileSync(file, 'utf-8');

    content = content.split('\n').map(s => s.replace(/^(import .+? from ["']\..+?)(["'];)$/, '$1.js$2')).join('\n');

    fs.writeFileSync(file, content, 'utf-8')
  });
});

@hugebdu
Copy link
Contributor

hugebdu commented Nov 6, 2022

any updates?

@shrpne
Copy link

shrpne commented Sep 14, 2023

@timostamm can we move it forward? without .js extension generated imports are broken for now

@bbeesley
Copy link

@timostamm any update on when this can be merged? What's holding it back at the moment?

@jan-dolejsi
Copy link

@timostamm , if you still cannot get your test pipeline to work with this, please consider releasing this as experimental. You can make it obvious that there is no guarantee, if you name it --experimental_enable_import_extensions for example. The ESM is taking over the world and we all would love this component to come along.

@nebez
Copy link

nebez commented Mar 11, 2024

Another version without npm dependencies while we await this feature to get merged. I use it as: buf generate && node ./patch-esm.js && tsc

#! /usr/bin/env node
import fs from 'node:fs';
import { dirname } from 'node:path';
import { fileURLToPath } from 'node:url';

const currentDir = dirname(fileURLToPath(import.meta.url));

const collectFiles = (directory, fileList = []) => {
    fs.readdirSync(directory)
        .map((innerFile) => `${directory}/${innerFile}`)
        .forEach((file) =>
            fs.statSync(file).isDirectory() ? collectFiles(file, fileList) : fileList.push(file)
        );
    return fileList;
};

const files = collectFiles(`${currentDir}/dist`).filter((file) => file.endsWith('.ts'));

files.forEach((file) => {
    const content = fs.readFileSync(file, 'utf8');
    const updated = content
        .split('\n')
        .map((s) => s.replace(/(} from ["']\..+(?<!\.js))(["'];)$/, '$1.js$2'))
        .join('\n');
    fs.writeFileSync(`${file}`, updated, 'utf-8');
});

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.

None yet