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

V8 engine support #739

Merged
merged 8 commits into from
Feb 25, 2020
Merged

Conversation

PopGoesTheWza
Copy link
Collaborator

Fixes #735

  • npm run test succeeds.
  • npm run lint succeeds.
  • Appropriate changes to README are included in PR.

@PopGoesTheWza PopGoesTheWza marked this pull request as ready for review February 22, 2020 01:00
@PopGoesTheWza
Copy link
Collaborator Author

@shinout : please review & test this PR

if OK, we'll ask grant for a release

Copy link

@shinout shinout left a comment

Choose a reason for hiding this comment

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

All your changes seem good to me 👍

One thing I would like to request is that the getTranspileOptions() in files.ts should use ts.findConfigFile(). Otherwise, tsconfig files cannot be found in monorepo-structured projects.

function getTranspileOptions(): ts.TranspileOptions {
  const projectPath = findUp.sync(DOT.PROJECT.PATH);
  const tsconfigPath = projectPath
    ? ts.findConfigFile(projectPath, ts.sys.fileExists)
    : undefined;
  if (!tsconfigPath) {
    return {};
  }
  const tsconfigContent = fs.readFileSync(tsconfigPath, FS_OPTIONS);
  const parsedConfigResult = ts.parseConfigFileTextToJson(
    tsconfigPath,
    tsconfigContent
  );
  return {
    compilerOptions: parsedConfigResult.config.compilerOptions
  };
}

This may work.

@shinout
Copy link

shinout commented Feb 22, 2020

@PopGoesTheWza
Also, I have to say that I failed to push ESNext codes to GAS even using this branch of clasp. As I repeatedly said, it's because of GAS server which parses pushed codes as non-ESNext ones. I think it's too early to release this version until GAS accepts ESNext codes via HTTP. Have you ever encountered a similar situation?

@PopGoesTheWza
Copy link
Collaborator Author

Otherwise, tsconfig files cannot be found in monorepo-structured projects.

@shinout do you have a sample monorepo to test?

I personally use this template for single repo/multiple GAS targets. I plan on updating it to support V8 engine.

@PopGoesTheWza
Copy link
Collaborator Author

until GAS accepts ESNext codes via HTTP

@shinout I am unsure what you mean. I had no time for testing clasp push of ES2019 yet but intend to do so this weekend.

@shinout
Copy link

shinout commented Feb 24, 2020

@PopGoesTheWza
Here is a sample monorepo directory structure.
This is the most typical one when using lerna.

(monorepo root)
├── tsconfig.json
├── packages
│   └── gas-package01
│       └── index.ts

The essence is that the main directory (gas-package01 here) is two or more directories distant from tsconfig.json. The current implementation only considers the parent directory of a given project. ts. findConfigFile() looks up ancestors recursively.
https://github.com/microsoft/TypeScript/blob/4a34294908bed6701dcba2456ca7ac5eafe0ddff/src/compiler/program.ts#L4

I am unsure what you mean

Sorry about that. Let me explain more precisely. The command clasp push uses googleapis's method projects.updateContent(). It accesses to this REST API, which seems to validate pushed codes before updating contents. The validation fails when we push ESNext codes, then the update cannot be done. I suppose the validation logic at their REST API server still doesn't accept ESNext grammar. Thus, even if this version of clasp is released, we won't be able to push ESNext codes.

@PopGoesTheWza PopGoesTheWza merged commit a204a44 into google:master Feb 25, 2020
@PopGoesTheWza PopGoesTheWza deleted the V8-engine-support branch February 25, 2020 19:43
@PopGoesTheWza
Copy link
Collaborator Author

@grant ready for release.

I drafted the release notes https://github.com/google/clasp/releases/edit/untagged-477cea24e1e150fb3fc4

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

Successfully merging this pull request may close these issues.

build option for V8 runtime
3 participants