edgedb-js icon indicating copy to clipboard operation
edgedb-js copied to clipboard

Replace imports of modules/cal with modules/std/cal

Open diksipav opened this issue 1 year ago • 4 comments

fixes #1119

From the research I did I only found the wrong cal imports inside range.ts .

diksipav avatar Oct 15 '24 10:10 diksipav

@scotttrinh , @jaclarke I'm really not sure should the driver behave differently depending on the server version, I am not sure does the driver should return std::cal::local_date etc as the typename of the codec or it is okay if it returns cal::local_date in the SCALAR_CODECS map also for server versions >=6?

@jaclarke just told me we don't use the typename info from the codecs anywhere so I guess we don't need to make any changes there. Or we can update cal:: with std::call just to be consistent with the current behavior instead with the "legacy". But in general, it doesn't matter.

diksipav avatar Oct 17 '24 10:10 diksipav

@scotttrinh I believe for the replace part inside edgeql-js.ts file you had sth like this in mind, but I don't think we really need this, because I believe in the future if we add anything we will use the correct path and also because std folder that is generated for example can have: ["cal", "enc", "fts", "math", "net"], but only cal, fts, math, and pg are actually moved, so I kinda think we do here unnecessary job...

  // write syntax files
  const syntaxOutDir = path.join(outputDir);
  if (!(await exists(syntaxOutDir))) {
    await fs.mkdir(syntaxOutDir);
  }

  const syntaxFiles = syntax[target];
  if (!syntaxFiles) {
    throw new Error(`Error: no syntax files found for target "${target}"`);
  }

  // libs that exists inside modules/std
  const stdLibs: string[] = [];

  if (version.major > 5) {
    const stdPath = path.join(prettyOutputDir, "modules", "std");
    const filenames = await fs.readdir(stdPath);

    for (const fname of filenames) {
      const fullPath = path.join(stdPath, fname);
      const fileStat = await fs.stat(fullPath);

      if (fileStat.isFile()) {
        const libName = path.parse(fname).name;
        stdLibs.push(libName);
      }
    }
  }

  for (const f of syntaxFiles) {
    const outputPath = path.join(syntaxOutDir, f.path);
    written.add(outputPath);

    const oldContents = await readFileUtf8(outputPath)
      .then((content) => content)
      .catch(() => "");

    let newContents = headerComment + f.content;

    // in server versions >=6 cal, fts, math and pg are moved inside std module
    if (version.major > 5) {
      stdLibs.forEach((lib) => {
        newContents = newContents.replace(
          `modules/${lib}`,
          `modules/std/${lib}`,
        );
      });
    }

    if (oldContents !== newContents) {
      await fs.writeFile(outputPath, newContents);
    }
  }

Screenshot 2024-10-17 at 13 03 25

diksipav avatar Oct 17 '24 12:10 diksipav

I believe for the replace part inside edgeql-js.ts file you had sth like this in mind, but I don't think we really need this, because I believe in the future if we add anything we will use the correct path and also because std folder that is generated for example can have: ["cal", "enc", "fts", "math", "net"], but only cal, fts, math, and pg are actually moved, so I kinda think we do here unnecessary job...

It would be necessary if we added a new type, similar to multirange, that needed access to, for instance, math. We would need to import from modules/math for <= 5 and /modules/std/math for >= 6. I know it would interate over some no-op values like enc and net (which never lived in the global module namespace), but that's a tiny price to pay to make this code generic enough to not have to hardcode specific values into. Since this is a generation-time cost, and hopefully the cost of running String.prototype.replace is pretty minor, I'd still prefer to do it this way.

If we want to optimize this and encode some of our knowledge here, we can just iterate over the modules we know got moved, but then I'd want to have a comment here explaining to someone coming after us that these modules used to live in the global module namespace and where moved, so we must special case them, and still iterate over all four of those modules (cal, fts, math, and pg).

scotttrinh avatar Oct 17 '24 13:10 scotttrinh

I'm really not sure should the driver behave differently depending on the server version, I am not sure does the driver should return std::cal::local_date etc as the typename of the codec or it is okay if it returns cal::local_date in the SCALAR_CODECS map also for server versions >=6?

I think we should return the unprefixed name for now since it works in both cases unless you've defined your own module with these exact module+type/function names. If we ever change the behavior here (not likely!) we can revisit. Or if users report issues where they're shadowing these modules on purpose and cannot get the TS driver to work, we can look into a workaround.

scotttrinh avatar Oct 17 '24 13:10 scotttrinh