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

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

Merged
merged 5 commits into from
Oct 21, 2024

Conversation

diksipav
Copy link
Contributor

@diksipav diksipav commented Oct 15, 2024

fixes #1119

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

@diksipav
Copy link
Contributor Author

diksipav commented Oct 17, 2024

@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
Copy link
Contributor Author

@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

@scotttrinh
Copy link
Collaborator

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
Copy link
Collaborator

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.


// in server versions >=6 cal, fts, math and pg are moved inside std module
if (f.path.split(".")[0] === "range" && version.major > 5) {
newContents = newContents.replace("modules/cal", "modules/std/cal");
Copy link
Member

@jaclarke jaclarke Oct 17, 2024

Choose a reason for hiding this comment

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

I guess this is unlikely to accidentally replace the wrong thing in the file, but maybe we should make this generate time replacement more explicit in the syntax files? Maybe by re-naming the mock modules dir we import from to something like MOCK_MODULES, then doing replacement on all modules imports that match the pattern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems like a good idea in general. I think we need to add some additional code to ensure that we don't copy the MOCK_MODULES directory at generate time because I think maybe the whole syntax dir is copied verbatim and then generation replaces stuff?

I don't think implementing this should be blocking though, we've already decided these fake modules existing is fine so this isn't any more confusing than what we had before 😅

Copy link
Contributor Author

@diksipav diksipav Oct 17, 2024

Choose a reason for hiding this comment

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

yes, I think this is how it works. I didn't update the PR with MOCK_MODULES rename and update...
if you think the rest is enough we can push this and if needed I can add further updates
I added comments that explains more why we do what we do

@diksipav diksipav force-pushed the 1119-move-libs-into-std branch from fcc7bea to f538f5f Compare October 17, 2024 15:45
@diksipav diksipav merged commit fb64e3c into master Oct 21, 2024
10 checks passed
@diksipav diksipav deleted the 1119-move-libs-into-std branch October 21, 2024 09:10
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.

The range module has a hard-coded path to cal module, but it is moving in 6.0
3 participants