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

Nodejs fixes #242

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

Nodejs fixes #242

wants to merge 5 commits into from

Conversation

wmertens
Copy link
Contributor

No description provided.

@wmertens
Copy link
Contributor Author

Note that the test failed due to memory issues while building prettier, which are unrelated to this PR

@wmertens
Copy link
Contributor Author

wmertens commented Aug 11, 2022

and the prettier build is failing because it has a build script, which I think is non-standard but the builder executes it because it's the main package. Plus I suppose it needs to build more than in other tests because the node version is now 16.

Would it not be better to require main packages to specify d2nBuild or use standard npm scripts?

config = ts.parseConfigFileTextToJson(data);

// https://www.typescriptlang.org/tsconfig#preserveSymlinks
config.compilerOptions.preserveSymlinks = true;
Copy link
Member

Choose a reason for hiding this comment

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

This logic is redundant with

${pkgs.jq}/bin/jq ".compilerOptions.preserveSymlinks = true" tsconfig.json \

@@ -14,7 +14,7 @@

changed = False

# fail if platform incompatible
# fail if platform incompatible - should not happen due to filters
Copy link
Member

Choose a reason for hiding this comment

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

Which filters do you mean?

@DavHau
Copy link
Member

DavHau commented Aug 16, 2022

and the prettier build is failing because it has a build script, which I think is non-standard but the builder executes it because it's the main package. Plus I suppose it needs to build more than in other tests because the node version is now 16.

We should either increase the --max-old-space value set here or simply update the version of prettier in the example (I guess).

Would it not be better to require main packages to specify d2nBuild or use standard npm scripts?

What do you mean by standard npm scripts? npm run build is the standard for building a package, or isn't it? Because that is exactly what we are executing by default.

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.

2 participants