feat(testlab): update sourceMappingURL when copying a JS file #951

virkt25 · 2018-02-01T18:39:12Z

When a .ts compled .js file is copied and required, a warning / error is logged to console because the accompanying .js.map file cannot be resolved. This PR updates the sourceMappingURL in the copied file to point to the original .js.map file as an absolute path so it can be resolved (only if accompanying .js.map file exists).

Based on a review comment here: #858 (comment)

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

kjdelisle

Okay, this PR looks simple enough, and I'm okay with the content, but let's timebox it at 30 minutes, or however long the first CI run is. If we can't get it all green by then, let's hang it up for later.

I want to avoid every non-MVP PR that we can.

kjdelisle

Actually, given that there are some bits I've overlooked in the TypeDoc (that I'm not sure of) I think this should wait until post-MVP.

kjdelisle · 2018-02-01T18:47:15Z

packages/testlab/src/test-sandbox.ts

+   * Copies a file from src to the TestSandbox. `.js.map` files are also copied
+   * to the sandbox by default.
+   * @param src Absolute path of file to be copied to the TestSandbox
+   * @param [dest] Optional. Destination path / filename of the copy operation


Shouldn't this be @param [string] dest Optional... ?

bajtos

This is great, @virkt25!

Please add a test to verify that copySourceCodeFile does not choke on error when there is no sourcemap file at the source.

bajtos · 2018-02-02T08:35:10Z

packages/testlab/src/test-sandbox.ts

+  async copySourceCodeFile(
+    src: string,
+    dest?: string,
+    copyMap: boolean = true,


In general: Boolean arguments are evil, please use an options object {copyMap: boolean} instead.

In this particular use case, I think copyMap option is YAGNI. Since your code is already handling the case where no .js.map file is available (and skip copying), I think this is all we need for now.

bajtos · 2018-02-02T08:38:52Z

packages/testlab/test/integration/test-sandbox.integration.ts

+  it('copies source file and map to directory', async () => {
+    await sandbox.copySourceCodeFile(TEST_FILE_RESOLVED);
+    expect(await pathExists(resolve(path, TEST_FILE))).to.be.True();
+    expect(await pathExists(resolve(path, TEST_FILE_MAP))).to.be.True();


These assertions will produce error messages that are rather unhelpful ("expected false to be true"). Please create a helper for verifying that a path exists, one that will include the file path in the error message.

For example:

async expectPathExists(filePath: string) { expect.ok( await pathExists(filePath), `Expected "${filePath}" to exist.`); }

bajtos · 2018-02-02T08:46:28Z

@kjdelisle

I want to avoid every non-MVP PR that we can.
Actually, given that there are some bits I've overlooked in the TypeDoc (that I'm not sure of) I think this should wait until post-MVP.

While I agree that we should avoid every non-MVP PR that we can (*), I disagree with the claim that this PR is non-MVP.

This particular pull request was created in response to my comment in #858 (comment) which does belong to MVP scope. While @virkt25 could make the changes proposed here in #858 (in which case you probably would not complain @kjdelisle , would you?), I appreciate that he did the right thing and opened a new PR where we can discuss those changes in isolation from the rest of that original PR that's already rather big.

My understanding of MVP is to keep the feature scope small, it is not an excuse for writing unclean code and introducing tech debt.

(*) I think we should consider an exception to that rule: pull requests improving our dev process (e.g. speeding our build-test cycles) should be allowed.

virkt25 · 2018-02-07T22:37:01Z

Some notes for future reference when this PR is worked on. Based on current usage of TestSandbox, the following will be very helpful and make using this utility much easier and cleaner.

.js.map files should not be re-named even if the actual file is renamed when copying the file over. (THIS IS VERY IMPORTANT).
Should consider introducing an API to allow us to copy multiple files at once.
All copy operations should support dest as just a folder name if a user doesn't want to rename the file.

bajtos · 2018-02-09T12:00:19Z

@virkt25 thank you for writing down the information for our future reference!

.js.map files should not be re-named even if the actual file is renamed when copying the file over. (THIS IS VERY IMPORTANT).

Wow, I find this rather surprising. Are you saying that after copy+rename, we will end up with foo.js and bar.js.map? Do you happen to know why is this needed? I would personally expect that foo.js should be accompanied by foo.js.map. Is it because foo.js contains a special comment pointing to bar.js.map and our copy+rename operation is not changing that comment?

virkt25 · 2018-02-11T06:34:40Z

The last line of the original .js files contains information for the accompanying .js.map file and the copy operation doesn't intend to modify the contents of the file itself. That can be an option I suppose.

An example last line in a compiled .js file. Even if a file is renamed, it still looks for original .js.map file.

//# sourceMappingURL=application.js.map

shimks

LGTM. One minor nitpick

shimks · 2018-04-09T18:12:01Z

packages/testlab/test/integration/test-sandbox.integration.ts

-  async function compareFiles(path1: string) {
-    const file = await readFile(path1, 'utf8');
-    expect(file).to.equal(fileContent);
+  async function compareFiles(original: string, copied: string) {


Nitpick: this function name should have the word expect in it so that it makes it clear that when the function is called it's doing an assertion. Something like expectFilesToBeIdentical

b-admike · 2018-04-10T20:16:31Z

packages/testlab/src/test-sandbox.ts

-   * Copies a file from src to the TestSandbox.
+   * Copies a file from src to the TestSandbox. If copying a `.js` file which
+   * has an accompanying `.js.map` file in the src file location, the dest file
+   * will have it's sourceMappingURL updated to point to the original file as


nitpick: it's -> its

virkt25 · 2018-04-11T15:12:30Z

@bajtos Please review when possible

bajtos

This is great, thank you @virkt25! I have one question to consider, see below.

bajtos · 2018-04-11T15:26:47Z

packages/testlab/src/test-sandbox.ts

+
+    if (parse(src).ext === '.js' && pathExists(src + '.map')) {
+      const srcMap = src + '.map';
+      await appendFile(dest, `\n//# sourceMappingURL=${srcMap}`);


IIUC, this assumes that the .js file does not contain any //# sourceMappingURL directive yet. Is it a safe assumption? Should we support the case when the .js file already contains sourceMappingURL directive?

Nope. A file can contain the directive but source-map only considers the last directive for the sourceMappingURL.

The moment we compile a file it gets this directive ... this adds the directive again with the absolute path of the original map file as the last line (so the previous line is ignored as just a comment) after copying the file over to it's new destination.

When a `.ts` compled `.js` file is copied and required, a warning / error is logged to console because the accompanying `.js.map` file cannot be resolved. This PR updates the sourceMappingURL in the copied file to point to the original `.js.map` file as an absolute path so it can be resolved (only if accompanying `.js.map` file exists).

virkt25 requested review from bajtos, kjdelisle and raymondfeng as code owners February 1, 2018 18:39

virkt25 added the needs-priority label Feb 1, 2018

kjdelisle approved these changes Feb 1, 2018

View reviewed changes

kjdelisle suggested changes Feb 1, 2018

View reviewed changes

virkt25 changed the title ~~feat(testlab): helper function to move .map files with .js files~~ [WIP] feat(testlab): helper function to move .map files with .js files Feb 1, 2018

virkt25 added the non-MVP label Feb 1, 2018

bajtos suggested changes Feb 2, 2018

View reviewed changes

bajtos mentioned this pull request Feb 2, 2018

feat: @loopback/boot #858

Merged

6 tasks

virkt25 requested review from b-admike, jannyHou and shimks as code owners April 6, 2018 23:17

virkt25 force-pushed the testlab-copy branch 2 times, most recently from 159a4bb to a513ccb Compare April 6, 2018 23:36

virkt25 changed the title ~~[WIP] feat(testlab): helper function to move .map files with .js files~~ feat(testlab): helper function to move .map files with .js files Apr 6, 2018

virkt25 changed the title ~~feat(testlab): helper function to move .map files with .js files~~ feat(testlab): update sourceMappingURL when copying a JS file Apr 6, 2018

virkt25 force-pushed the testlab-copy branch from a513ccb to 1fb6e36 Compare April 9, 2018 17:55

shimks approved these changes Apr 9, 2018

View reviewed changes

virkt25 force-pushed the testlab-copy branch from 1fb6e36 to 653c159 Compare April 10, 2018 19:24

b-admike approved these changes Apr 10, 2018

View reviewed changes

virkt25 force-pushed the testlab-copy branch 2 times, most recently from 94a1c51 to 63edd51 Compare April 11, 2018 15:05

bajtos approved these changes Apr 11, 2018

View reviewed changes

virkt25 force-pushed the testlab-copy branch from 63edd51 to 229ef8c Compare April 11, 2018 17:47

virkt25 merged commit aac2781 into master Apr 11, 2018

virkt25 deleted the testlab-copy branch April 11, 2018 17:55

virkt25 removed the needs-priority label Apr 11, 2018

dhmlau mentioned this pull request Apr 30, 2018

Monthly Milestone - April 2018 #1044

Closed

23 tasks

feat(testlab): update sourceMappingURL when copying a JS file #951

feat(testlab): update sourceMappingURL when copying a JS file #951

Uh oh!

Conversation

virkt25 commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

kjdelisle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjdelisle left a comment

Choose a reason for hiding this comment

Uh oh!

kjdelisle Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

bajtos Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

bajtos commented Feb 2, 2018

Uh oh!

virkt25 commented Feb 7, 2018

Uh oh!

bajtos commented Feb 9, 2018

Uh oh!

virkt25 commented Feb 11, 2018

Uh oh!

shimks left a comment

Choose a reason for hiding this comment

Uh oh!

shimks Apr 9, 2018

Choose a reason for hiding this comment

Uh oh!

b-admike Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

virkt25 commented Apr 11, 2018

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

virkt25 Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

virkt25 commented Feb 1, 2018 •

edited

Loading

kjdelisle left a comment •

edited

Loading