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

Added ability in watch mode to copy source maps to output folder for … #250

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,11 @@ public boolean getSourcemapsEnabled() {
return Boolean.parseBoolean(getString("enableSourcemaps"));
}

@Override
public boolean getEnableIncrementalSourcemaps() {
return Boolean.parseBoolean(getString("enableIncrementalSourcemaps"));
}

@Override
public String getInitialScriptFilename() {
return getString("initialScriptFilename");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ public interface Config {

boolean getSourcemapsEnabled();

boolean getEnableIncrementalSourcemaps();

String getInitialScriptFilename();

Map<String, String> getDefines();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,12 @@ public class BuildMojo extends AbstractBuildMojo {
@Parameter(defaultValue = "false")
protected boolean enableSourcemaps;

/**
* True to copy only changed source files to the output directory. This is a performance optimization.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* True to copy only changed source files to the output directory. This is a performance optimization.
* True to copy only changed source files to the output directory. This is a performance optimization, but could result in incorrect output if generated files are altered outside of this plugin.

Copy link
Member

Choose a reason for hiding this comment

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

...or if output is cached from an earlier run.

Basically you are sacrificing correctness for speed, especially if your "speed" also comes in the form of "i made it faster by using a cache outside of target".

*/
@Parameter(defaultValue = "false")
protected boolean enableIncrementalSourcemaps;

@Override
public void execute() throws MojoExecutionException, MojoFailureException {
// pre-create the directory so it is easier to find up front, even if it starts off empty
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,12 @@ public class TestMojo extends AbstractBuildMojo {
@Parameter(defaultValue = "false")
protected boolean enableSourcemaps;

/**
* True to copy only changed source files to the output directory. This is a performance optimization.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* True to copy only changed source files to the output directory. This is a performance optimization.
* True to copy only changed source files to the output directory. This is a performance optimization, but could result in incorrect output if generated files are altered outside of this plugin.

*/
@Parameter(defaultValue = "false")
protected boolean enableIncrementalSourcemaps;

/**
* Closure flag: "Source of translated messages. Currently only supports XTB."
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,12 @@ public class WatchMojo extends AbstractBuildMojo {
@Parameter(defaultValue = "true")
protected boolean enableSourcemaps;

/**
* True to copy only changed source files to the output directory. This is a performance optimization.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* True to copy only changed source files to the output directory. This is a performance optimization.
* True to copy only changed source files to the output directory. This is a performance optimization, but could result in incorrect output if generated files are altered outside of this plugin.

*/
@Parameter(defaultValue = "false")
protected boolean enableIncrementalSourcemaps;

/**
* @deprecated Will be removed in 0.21
*/
Expand Down
2 changes: 1 addition & 1 deletion j2cl-tasks/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,4 @@
<scope>test</scope>
</dependency>
</dependencies>
</project>
</project>
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ public Task resolve(Project project, Config config) {
}

File initialScriptFile = config.getWebappDirectory().resolve(config.getInitialScriptFilename()).toFile();
boolean sourcemapsEnabled = config.getSourcemapsEnabled() && !config.getEnableIncrementalSourcemaps();

Map<String, Object> defines = new LinkedHashMap<>(config.getDefines());

List<Input> outputToCopy = Stream.concat(
Expand Down Expand Up @@ -113,11 +115,14 @@ public void finish(TaskContext taskContext) throws IOException {
Files.copy(bundle.getAbsolutePath(), targetFile, StandardCopyOption.REPLACE_EXISTING);
}

File destSourcesDir = outputDir.toPath().resolve(Closure.SOURCES_DIRECTORY_NAME).toFile();
destSourcesDir.mkdirs();
for (Path dir : jsSources.stream().map(Input::getParentPaths).flatMap(Collection::stream).map(p -> p.resolve(Closure
.SOURCES_DIRECTORY_NAME)).collect(Collectors.toSet())) {
FileUtils.copyDirectory(dir.toFile(), destSourcesDir);
// Copy sources to the output directory, if sourcemaps are enabled and incremental source maps is not.
if(sourcemapsEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(sourcemapsEnabled) {
if (sourcemapsEnabled) {

File destSourcesDir = outputDir.toPath().resolve(Closure.SOURCES_DIRECTORY_NAME).toFile();
destSourcesDir.mkdirs();
for (Path dir : jsSources.stream().map(Input::getParentPaths).flatMap(Collection::stream).map(p -> p.resolve(Closure
.SOURCES_DIRECTORY_NAME)).collect(Collectors.toSet())) {
FileUtils.copyDirectory(dir.toFile(), destSourcesDir);
}
}

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ public Task resolve(Project project, Config config) {

// Consider treating this always as true, since the build doesnt get more costly to be incremental
boolean incrementalEnabled = config.isIncrementalEnabled();
boolean enableIncrementalSourcemaps = config.getEnableIncrementalSourcemaps();

Gson gson = new GsonBuilder().setPrettyPrinting().serializeNulls().create();

Expand Down Expand Up @@ -132,6 +133,7 @@ public Task resolve(Project project, Config config) {
depInfoMap = deps.stream()
.map(info -> new DependencyInfoAndSource(
info,
fileNameKey,
() -> Files.readString(lastOutput.resolve(Closure.SOURCES_DIRECTORY_NAME).resolve(info.getName())))
)
.collect(Collectors.toMap(DependencyInfo::getName, Function.identity()));
Expand All @@ -151,7 +153,7 @@ public Task resolve(Project project, Config config) {
input.setCompiler(jsCompiler);
depInfoMap.put(
change.getSourcePath().toString(),
new DependencyInfoAndSource(input, input::getCode)
new DependencyInfoAndSource(input, fileNameKey, input::getCode)
);
}
}
Expand All @@ -170,8 +172,7 @@ public Task resolve(Project project, Config config) {
.withOriginalPath(path.getSourcePath().toString())
.build());
input.setCompiler(jsCompiler);

dependencyInfos.add(new DependencyInfoAndSource(input, input::getCode));
dependencyInfos.add(new DependencyInfoAndSource(input, fileNameKey, input::getCode));
}
}
}
Expand Down Expand Up @@ -202,6 +203,7 @@ public Task resolve(Project project, Config config) {
for (DependencyInfoAndSource info : sorter.getSortedList()) {
String code = info.getSource();
String name = info.getName();
String projectName = info.getProject();

//TODO do we actually need this?
if (Compiler.isFillFileName(name) && code.isEmpty()) {
Expand All @@ -210,7 +212,8 @@ public Task resolve(Project project, Config config) {

// append this file and a comment where it came from
bundleOut.append("//").append(name).append("\n");
bundler.withPath(name).withSourceUrl(Closure.SOURCES_DIRECTORY_NAME + "/" + name).appendTo(bundleOut, info, code);
String sourceMapUrl = Closure.SOURCES_DIRECTORY_NAME + (enableIncrementalSourcemaps ? ("/" + projectName) : "") + "/" + name;
bundler.withPath(name).withSourceUrl(sourceMapUrl).appendTo(bundleOut, info, code);
bundleOut.append("\n");

}
Expand Down Expand Up @@ -245,8 +248,11 @@ public static class DependencyInfoAndSource implements DependencyInfo {
private final DependencyInfo delegate;
private final SourceSupplier sourceSupplier;

public DependencyInfoAndSource(DependencyInfo delegate, SourceSupplier sourceSupplier) {
private final String project;

public DependencyInfoAndSource(DependencyInfo delegate, String project, SourceSupplier sourceSupplier) {
this.delegate = delegate;
this.project = project.replaceAll("\\.", "-");
this.sourceSupplier = sourceSupplier;
}

Expand Down Expand Up @@ -309,6 +315,10 @@ public boolean getHasExternsAnnotation() {
public boolean getHasNoCompileAnnotation() {
return delegate.getHasNoCompileAnnotation();
}

public String getProject() {
return project;
}
}

public static class DependencyInfoFormat implements DependencyInfo {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@

import com.google.auto.service.AutoService;
import com.google.j2cl.common.SourceUtils;
import com.google.javascript.jscomp.CompilationLevel;
import com.vertispan.j2cl.build.task.*;
import com.vertispan.j2cl.tools.Closure;
import com.vertispan.j2cl.tools.J2cl;
import org.apache.commons.io.FileUtils;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.PathMatcher;
import java.util.Collections;
Expand Down Expand Up @@ -53,6 +58,10 @@ public Task resolve(Project project, Config config) {

File bootstrapClasspath = config.getBootstrapClasspath();
List<File> extraClasspath = config.getExtraClasspath();

boolean sourcemapsEnabled = config.getSourcemapsEnabled() && config.getEnableIncrementalSourcemaps();
Path sourceMapsFolder = sourcemapsEnabled ? prepareSourcesFolder(config, project) : null;

return context -> {
if (ownJavaSources.getFilesAndHashes().isEmpty()) {
return;// nothing to do
Expand Down Expand Up @@ -84,6 +93,26 @@ public Task resolve(Project project, Config config) {
if (!j2cl.transpile(javaSources, nativeSources)) {
throw new IllegalStateException("Error while running J2CL");
}

if(sourcemapsEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(sourcemapsEnabled) {
if (sourcemapsEnabled) {

if(sourceMapsFolder.toFile().exists()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(sourceMapsFolder.toFile().exists()) {
if (sourceMapsFolder.toFile().exists()) {

FileUtils.deleteDirectory(sourceMapsFolder.toFile());
}
sourceMapsFolder.toFile().mkdirs();
FileUtils.copyDirectory(context.outputPath().toFile(), sourceMapsFolder.toFile());
}
};
}

private Path prepareSourcesFolder(Config config, Project project) {
Copy link
Member

Choose a reason for hiding this comment

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

Won't doing this here prevent projects that don't get j2cl run on them (bootstrap, jre, any JS-only dependency) from getting sourcemaps? It seems like this might belong in ClosureBundleTask instead (copy from "here's where my input declared its sourcemaps, skip ahead and copy straight to the output dir).

That would also remove the need to have a "is compilationLevel==BUNDLE_JAR", which technically this should have as written - if you have this property on when doing a prod build, you're going to write a bunch of extra stuff to prod that you probably don't want...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Won't doing this here prevent projects that don't get j2cl run on them (bootstrap, jre, any JS-only dependency) from getting sourcemaps? It seems like this might belong in ClosureBundleTask instead (copy from "here's where my input declared its sourcemaps, skip ahead and copy straight to the output dir).

That would also remove the need to have a "is compilationLevel==BUNDLE_JAR", which technically this should have as written - if you have this property on when doing a prod build, you're going to write a bunch of extra stuff to prod that you probably don't want...

Definitely, what's good is that the projects you mentioned are those that won't change during the j2cl:watch process, which means their source maps need to be copied only once. ClosureBundleTask is definitely the best place for this.

I was thinking about how to copy the source maps of reactor modules in ClosureBundleTask, rather than in J2clTask, but I couldn't figure out how to let ClosureBundleTask know which projects have changed and which have not. Maybe you have some ideas?

try {
Path initialScriptFile = config.getWebappDirectory().resolve(config.getInitialScriptFilename());
Path destSourcesDir = Files.createDirectories(initialScriptFile.getParent()).resolve(Closure.SOURCES_DIRECTORY_NAME);
return Files.createDirectories(destSourcesDir).resolve(project.getKey()
.replaceAll("\\.","-")
.replaceAll(":","-"));
} catch (IOException e) {
throw new RuntimeException(e);
}
}
}
Loading