feat(compiler): consumer sourcemap support #3005

rwaskiewicz · 2021-08-17T20:15:39Z

This PR is an exact copy of #2892. After Prettier was added to the codebase, merge conflicts arose despite our best efforts. I took some time to clear them out on a separate branch. But a large majority of the work here is attributed to @johnjenkins, without whom we would not be nearly this far along.

This branch continues that work, as the Stencil team continues to refine sourcemap work and prepare it to be merged into the main branch.

Note: We should validate the issues that this closes by leveraging the list that John has created in their PR summary

fixes #1744
fixes #1650
fixes #219
fixes #1255

rwaskiewicz · 2021-08-17T20:29:46Z

src/compiler/build/compiler-ctx.ts

@@ -103,6 +103,8 @@ export const getModuleLegacy = (_config: d.Config, compilerCtx: d.CompilerCtx, s
      potentialCmpRefs: [],
      staticSourceFile: null,
      staticSourceFileText: '',
+      sourceMapPath: null,


Ryan: Consider why we don't put this in the build context, as that gets refreshed on every new build

Maybe it's because these things are static?

This may be a signal that a rename of these fields may be necessary

AFAIK, this method helps build the moduleMap, I don't understand yet why it's named with "legacy" yet, but generally this should help to build the object that will ultimately be a part of the moduleMap.

Interestingly enough, this is in some way already on the buildCtx too, since buildCtx takes in the compilerCtx and uses that to do some internal stuff throughout the life of a build. So, this is technically available on the buildCtx, and may in fact be used by it too, but I don't yet know if it is for sure. At a cursory search through the codebase, it doesn't seem to be used. (I searched with .compilerCtx.moduleMap)

I assume it's set up this way to save on memory. If the module map was instantiated and populated for each build, there would be a lot of processing power and disk reads that would be prohibitive. So having it at a higher level cache and providing it down to the buildCtx should help mitigate that.

I swear I'll get around to an ADR on compilerCtx and buildCtx lol. There's a lot to unravel about these two concepts.

rwaskiewicz · 2021-08-17T20:35:09Z

src/compiler/bundle/app-data-plugin.ts

@@ -11,6 +11,8 @@ import {
  STENCIL_INTERNAL_HYDRATE_ID,
 } from './entry-alias-ids';
 import ts from 'typescript';
+import { basename } from 'path';


Ryan: Should this be moved to the Node-Sys? We need to see if this is called in a Node context at all times or not?

I think we should use the abstraction here.

I took a look into this this AM, turns out that we do our own little polyfill of path - there isn't a node-sys version of path. This is fine as-is

rwaskiewicz · 2021-08-17T20:57:34Z

src/compiler/bundle/app-data-plugin.ts

+      }
+
+      const module = compilerCtx.moduleMap.get(config.globalScript);
+      if (!module.sourceMapFileText) return { code: module.staticSourceFileText, map: null };


Should we validate that module exists here?

Suggested change

if (!module.sourceMapFileText) return { code: module.staticSourceFileText, map: null };

if (!module) {

return null;

}

if (!module.sourceMapFileText) return { code: module.staticSourceFileText, map: null };

I think this is a good idea imho.

Should we resolve this?

Not yet, let me put up a PR with a few tweaks to this file

src/compiler/bundle/app-data-plugin.ts

rwaskiewicz · 2021-08-17T21:17:40Z

src/compiler/bundle/app-data-plugin.ts

+
+      const sourceMap: d.SourceMap = JSON.parse(module.sourceMapFileText);
+      sourceMap.sources = sourceMap.sources.map((src) => basename(src));
+      return { code: module.staticSourceFileText, map: sourceMap };


TODO I want to investigate this further with respect to these fields (they aren't the ones that were added in the PR)

I think this is the signature of the LoadResult as a SourceDescription

Similarly for a rollup plugin's transform method - TransformResult as a partial Source Description

splitinfinities · 2021-08-19T15:32:03Z

src/compiler/bundle/app-data-plugin.ts

@@ -52,7 +54,7 @@ export const appDataPlugin = (
      return null;
    },

-    load(id) {
+    load(id): d.RollupLoadHook {


I've been thinking a lot on this file.

I want to propose a couple refactoring items here that are not blocking, aren't worth adding to this PR, but worth talking about and adding a ticket to Jira if agreed upon.

First, I think we should try to set a policy around our internal rollup plugins and their load/transform to always return either null, or the SourceDescription object. That way, extending this code to allow source maps to other new files later down the line aren't a problem, but also if we ever want to pass along an AST without a source map, we can do just that.

I don't necessarily agree about sometimes returning a string, or sometimes returning an object, and would love to see that as a policy for these two methods.

I rewrote this function to be:

load(id) { const s = new MagicString(``); let result = null; if (id === STENCIL_APP_GLOBALS_ID) { appendGlobalScripts(globalScripts, s); result = { code: s.toString(), map: undefined, }; } else if (id === STENCIL_APP_DATA_ID) { appendNamespace(config, s); appendBuildConditionals(config, build, s); appendEnv(config, s); result = { code: s.toString(), map: undefined, }; } else if (id === config.globalScript) { const module = compilerCtx.moduleMap.get(config.globalScript); if (!module.sourceMapFileText) return { code: module.staticSourceFileText, map: null }; const sourceMap: d.SourceMap = JSON.parse(module.sourceMapFileText); sourceMap.sources = sourceMap.sources.map((src) => basename(src)); result = { code: module.staticSourceFileText, map: sourceMap }; } return result; },

Second, and more contentious because I don't have a deeper understanding of the flow of data - I think in an effort to remove the if statements from the plugin, we should abstract this into three separate plugins.

the STENCIL_APP_GLOBALS_ID plugin

the STENCIL_APP_DATA_ID plugin

the config.globalScript plugin

That way this file can be kept small and testable.

Meaning, we'd have three plugins with these load functions:

app-data-globals-plugin.ts

load(id) { const s = new MagicString(``); let result = null; if (id === STENCIL_APP_GLOBALS_ID) { appendGlobalScripts(globalScripts, s); result = { code: s.toString(), map: undefined, }; } return result; },

app-data-plugin.ts

load(id) { const s = new MagicString(``); let result = null; if (id === STENCIL_APP_DATA_ID) { appendNamespace(config, s); appendBuildConditionals(config, build, s); appendEnv(config, s); result = { code: s.toString(), map: undefined, }; } return result; },

app-data-globalScript-plugin.ts

load(id) { let result = null; if (id === config.globalScript) { const module = compilerCtx.moduleMap.get(config.globalScript); if (!module.sourceMapFileText) return { code: module.staticSourceFileText, map: null }; const sourceMap: d.SourceMap = JSON.parse(module.sourceMapFileText); sourceMap.sources = sourceMap.sources.map((src) => basename(src)); result = { code: module.staticSourceFileText, map: sourceMap }; } return result; },

I recognize this is a subset of the content of the original file, so this is naive. But this plugin is overloaded without a doubt.

For future reference - I think passing around an AST instead of a string may improve the performance of our consumers compilation. See rollup/rollup#782

rwaskiewicz · 2021-08-23T17:15:18Z

package.json

@@ -99,6 +99,7 @@
    "jest-environment-node": "^26.6.2",
    "listr": "^0.14.3",
    "magic-string": "^0.25.7",
+    "merge-source-map": "^1.1.0",


I have a question out to see if this needs to be added to

stencil/scripts/license.ts

Line 9 in 925d4e9

const entryDeps = [

or not

NVM - got confirmation we do (need to add it) - #3016

Done https://github.com/ionic-team/stencil/pull/3005/files#diff-a559fe1aaf8746e145121786b79b8b86b1f87b4da31f52039453bfb12b8370f8R21

rwaskiewicz · 2021-08-25T17:38:24Z

src/utils/sourcemaps.ts

+
+export const rollupSrcMapToObj = (rollupSrcMap: RollupSourceMap): d.SourceMap => {
+  if (!rollupSrcMap) return null;
+  if (typeof rollupSrcMap.toUrl === 'function') return JSON.parse(rollupSrcMap.toString());


Question: If rollupSrcMap is of type RollupSourceMap, which is defined as:

export interface SourceMap { ... toUrl(): string; }

when is this case false?

rwaskiewicz · 2021-08-25T17:38:46Z

src/utils/sourcemaps.ts

+export const rollupSrcMapToObj = (rollupSrcMap: RollupSourceMap): d.SourceMap => {
+  if (!rollupSrcMap) return null;
+  if (typeof rollupSrcMap.toUrl === 'function') return JSON.parse(rollupSrcMap.toString());
+  if (typeof rollupSrcMap === 'string') return JSON.parse(rollupSrcMap);


Question: If rollupSrcMap is of type RollupSourceMap, which is defined as:

export interface SourceMap { ... }

when is this case true?

rwaskiewicz · 2021-08-25T17:39:48Z

src/utils/sourcemaps.ts

+  if (!rollupSrcMap) return null;
+  if (typeof rollupSrcMap.toUrl === 'function') return JSON.parse(rollupSrcMap.toString());
+  if (typeof rollupSrcMap === 'string') return JSON.parse(rollupSrcMap);
+  return rollupSrcMap;


Note - I'm gonna dig more into this one. This feels like it could be brittle, as it assumes a level of parity between our Sourcemap representation and Rollup's. But I need to dig in a bit more

I think your observations are quite right.
I have vague recollections of initially using this function to turn a string OR RollupSourceMap to a sourceMap proper.
But later changed; only using it to convert the rollup variant so these checks became obsolete.
I think I changed the name of the function but kept the checks in - either 'cos I forgot or thought it didn't do any harm

That makes sense, thanks! I'm gonna dig into just to be sure 🙂

rwaskiewicz · 2021-08-27T17:42:51Z

src/compiler/output-targets/dist-lazy/generate-cjs.ts

@@ -21,6 +21,7 @@ export const generateCjs = async (
      entryFileNames: '[name].cjs.js',
      assetFileNames: '[name]-[hash][extname]',
      preferConst: true,
+      sourcemap: !!config.sourceMap,


Note for future us: Someday I'd like to move us to a model where we bifurcate the idea of a Stencil config into two internally - one model for unvalidated user input, and other for validated configuration. We do this to some degree today (we surely validate the config) - but a separate interface would allow us to narrow the types and avoid things like this moving forward.

…t-custom-elements, dist, esm, esm-es5 and cjs. Added option to StencilConfig type options.

this PR replaces our custom typings for rollup hooks with those provided by rollup

add merge-source-map to license file list, as it will be included in the Stencil distributable

simplify the transformation of sourcemaps by removing code paths that I don't believe will be followed. add tests to the sourcemaps utility function

ensure that upon validating a user's stencil config, the value of the 'sourcemap' field is never undefined. remove unneeded 'bang bang' operators after forcing souremap to boolean

this commit refactors & renames `src/compiler/config/test/validate-ts-sourcemap.spec.ts` to `src/compiler/config/test/validate-config-sourcemap.spec.ts` the file was renamed to align with the unit test file `validate-config.spec.ts`, found in the same directory. This file was not merged with `validate-config.spec.ts` to avoid more refactoring than necessary. the refactoring of the test file consists of: - renaming the suite & associated tests for clarity - removing unnecessary filesystem calls - refactoring generating a `LoadConfigInit` entity out to a helper fn - variable renaming

this PR makes changes to the Stencil-defined type declaration file for `merge-source-map` `merge-source-map` does not offer any `d.ts` file, nor does it have a third party offering on Definitely Typed, so the intent of this effort was to see "how correct is our type definition for our use case, while not deviating from the actual library's implicit typings".

rwaskiewicz · 2021-09-21T18:10:09Z

src/compiler/bundle/app-data-plugin.ts

+            includeContent: true,
+            hires: true,
+          });
+          return { code: results.outputText, map: sourceMapMerge(codeMap, sourceMap) };


@johnjenkins Do you recall why we want to merge the sourcemaps here?

I did some digging, at comparing the sourcemap generated by the TS compiler (left in the image below) and the one generated by MagicString (right in the image below):

It seems the only difference is the name of file and mappings.

I think per the spec for sourcemaps, file should point to globals.js in this situation (and should be easy enough to handle).

The hires (high resolution) mappings are what I'm intrigued by. I took at look at the MagicString Docs

whether the mapping should be high-resolution. Hi-res mappings map every single character, meaning (for example) your devtools will always be able to pinpoint the exact location of function calls and so on. With lo-res mappings, devtools may only be able to identify the correct line - but they're quicker to generate and less bulky. If sourcemap

Was the idea here to get sourcemaps with higher fidelity, then merge them back into the ones generated by TS?

Hi @rwaskiewicz - yes the difference in mappings is the important thing ...
We need to merge sourcemaps because we need to account for the extra import / export strings of code that can get added and to this end I needed to get creative!

Turn the incoming TS string to a MagicString - this will track the changes we make to the original TS source string and can generate a sourcemap tracking these changes.

Transpile the TS using the transpileModule - this generates another sourcemap however the typescript compiler has no idea of the earlier amends made to the source TS therefore the sourcemap generated does not reflect these amends and is incorrect.

Merge the original sourcemap from MagicString and the new one from typescript - all changes made to the original source now being accounted for.

This is a little convoluted I agree - I was just working with what was there originally and appending strings of code to source input should be avoided in general for this reason.
For future devs - try to make these amends to sourcecode within TS plugins / AST to avoid this hinkiness.

Regarding the hires property - I simply applied it because the weird process described and corresponding merge would produce slightly incorrect sourcemaps and this fixed it.
Attached is 2 screen caps. 1 with hires - each console points to the correct line from the original source. 2 without hires source lines highlighted can be out by 1 (or 2)
https://user-images.githubusercontent.com/5030133/134250837-edc67824-f668-4b85-8b34-4966017cc70c.mov
https://user-images.githubusercontent.com/5030133/134250827-634f7e57-5e24-451c-a353-21dde518489b.mov

this commit adds additional typings to `generateRollupOutput` to help make it easier to understand the map() function call that is invoked on the results of calling `build.generate()` sourcemap is an optional property on rollup source maps, update our representation extract the labels used in timespans to variables, to make it less likely we'll mess up their content in two different callsites this commit also adds return types to the methods updated

duplicate the typings for rollup (again). Stencil cannot resolve the rollup typings in the private compiler api typings at run time for consuming projects. As a result, we duplicate the sourcemap typings from rollup partial revert of #3009

splitinfinities

Looking good! Going to try this out in a couple local libraries and circle back.

splitinfinities · 2021-10-05T18:02:24Z

src/compiler/app-core/bundle-app-core.ts

-  const { output } = await build.generate(options);
-  return output.map((chunk) => {
+  const { output }: { output: [OutputChunk, ...(OutputChunk | OutputAsset)[]] } = await build.generate(options);
+  return output.map((chunk: OutputChunk | OutputAsset) => {


For my own understanding, what the difference between these two - Chunk and Asset?

splitinfinities · 2021-10-05T18:03:38Z

src/compiler/bundle/app-data-plugin.ts

+      }
+
+      const module = compilerCtx.moduleMap.get(config.globalScript);
+      if (!module.sourceMapFileText) return { code: module.staticSourceFileText, map: null };


Should we resolve this?

splitinfinities · 2021-10-05T18:10:35Z

src/declarations/stencil-private.ts

@@ -346,6 +356,13 @@ export interface BundleEntryInputs {
  [entryKey: string]: string;
 }

+/**
+ * A note regarding Rollup types:


@rwaskiewicz do you consider this Technical debt? Would we have a ticket in the backlog already if so?

I don't think so, it's an archtectural aspect of the system (I think, in so far as I can tell rn). I'll add something to the backlog as a low priority spike

STENCIL-175

add a check to verify that a global script exists in the moduleMap on the compiler context before accessing fields on it to prevent compiler time errors. this is one of the items we discussed as a team in the GH comments of #3005, and is being implemented here reworked how sourcemaps are generated. this commit restores the original code generation where the platform, source code, and default export are concatenated, which allows us to reserve generating a magic string until/if we're using source maps remove includeContent field from the generation of the sourcemap, which does not appear to be needed, strictly speaking

this commit introduces a new type, OptimizeJsResult, to be used to name the shape of the object that is returned by various methods that are used to minify a user's JavaScript. using this type in the return signature of methods + in the declaration of objects allows us to eliminate unnessary type assertions on the individual fields of said objects. * add JSDoc to minification to the functions that were investigated as a part of this effort. this is done in an effort to make the dissemination of code within Stenicl slightly better. of course, we incur the cost of maintaining these comments. * account for minification with sourcemaps sourcemaps in some regard are at odds with attempting to minize and compress our JavaScript files. Ideally there's no fidelity lost when compressing JS, but in some cases, we get inconsistent behaviors across browsers when we have both 'sourcemap' and 'compress' enabled in our terser configuration. after diffing through the terser source/docs, I think our best route forward for now is to simply disable the compress step. I took a sample Stencil app and did a `diff` of the output where in one scenario we didn't set compress to undefined, and one where we did set it to undefined. other than the sourcemaps themselves, the differences were rather minimal, and I believe this shouldn't have a large impact on the output

add a check to verify that a global script exists in the moduleMap on the compiler context before accessing fields on it to prevent compiler time errors. this is one of the items we discussed as a team in the GH comments of #3005, and is being implemented here reworked how sourcemaps are generated. this commit restores the original code generation where the platform, source code, and default export are concatenated, which allows us to reserve generating a magic string until/if we're using source maps remove includeContent field from the generation of the sourcemap, which does not appear to be needed, strictly speaking

add a check to verify that a global script exists in the moduleMap on the compiler context before accessing fields on it to prevent compiler time errors. this is one of the items we discussed as a team in the GH comments of #3005, and is being implemented here reworked how sourcemaps are generated. this commit restores the original code generation where the platform, source code, and default export are concatenated, which allows us to reserve generating a magic string until/if we're using source maps

johnjenkins

LGTM

splitinfinities

Looks and works great! AWESOME work @johnjenkins and @rwaskiewicz!!

this commit adds sourcemap functionality for consumers of the following output targets: - dist-custom-elements - dist-custom-elements-bundle - www source maps are produced for JavaScript files that are emitted by running the Stencil compiler. this functionality is off by default, and must be enabled by setting the `sourceMap: true` flag in the root of a project's Stencil configuration file object

fohlin · 2021-10-07T07:29:37Z

Great work everyone! 👏 💯

LukePammant · 2021-10-07T15:53:21Z

Woohoo can't wait to use this! Thanks for all effort!

theo-staizen · 2021-10-12T10:25:09Z

Beautiful work lads, just beautiful <3

rwaskiewicz added the Feature: Sourcemaps label Aug 18, 2021

rwaskiewicz commented Aug 18, 2021

View reviewed changes

splitinfinities reviewed Aug 19, 2021

View reviewed changes

rwaskiewicz commented Aug 23, 2021

View reviewed changes

rwaskiewicz marked this pull request as ready for review August 24, 2021 17:19

rwaskiewicz requested a review from a team August 24, 2021 17:19

rwaskiewicz marked this pull request as draft August 24, 2021 17:20

splitinfinities assigned splitinfinities and rwaskiewicz Aug 24, 2021

rwaskiewicz commented Aug 25, 2021

View reviewed changes

rwaskiewicz commented Aug 27, 2021

View reviewed changes

rwaskiewicz mentioned this pull request Aug 27, 2021

chore(config): coerce sourceMap config value to boolean #3035

Merged

12 tasks

gavindoughtie-aon mentioned this pull request Sep 8, 2021

feat(compiler): source maps #2892

Closed

12 tasks

John Jenkins and others added 7 commits September 20, 2021 08:37

Sourcemaps generated for collection, dist-custom-elements-bundle, dis…

fcc8dc3

…t-custom-elements, dist, esm, esm-es5 and cjs. Added option to StencilConfig type options.

chore(types) remove bespoke rollup types (#3009)

86ddc5d

this PR replaces our custom typings for rollup hooks with those provided by rollup

add merge-source-map to license file list (#3016)

343e418

add merge-source-map to license file list, as it will be included in the Stencil distributable

chore(sourcemaps): simplify transformation of sourcemaps (#3026)

24d7de4

simplify the transformation of sourcemaps by removing code paths that I don't believe will be followed. add tests to the sourcemaps utility function

chore(config): coerce sourceMap config value to boolean (#3035)

6fb59d1

ensure that upon validating a user's stencil config, the value of the 'sourcemap' field is never undefined. remove unneeded 'bang bang' operators after forcing souremap to boolean

rwaskiewicz force-pushed the rwaskiewicz-rebase-again-sourcemaps branch from e8cab1d to 7ff03dc Compare September 20, 2021 12:38

rwaskiewicz commented Sep 21, 2021

View reviewed changes

rwaskiewicz mentioned this pull request Sep 22, 2021

chore(sourcemaps): create linker string util #3077

Merged

14 tasks

rwaskiewicz added 3 commits September 23, 2021 15:59

chore(sourcemaps): create linker string util (#3077)

39f7356

splitinfinities added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Oct 5, 2021

splitinfinities reviewed Oct 5, 2021

View reviewed changes

rwaskiewicz mentioned this pull request Oct 6, 2021

chore(sourcemaps): update global script handling #3094

Merged

14 tasks

rwaskiewicz added 2 commits October 6, 2021 11:10

Merge branch 'master' into rwaskiewicz-rebase-again-sourcemaps

4a89eb0

rwaskiewicz marked this pull request as ready for review October 6, 2021 15:41

rwaskiewicz requested a review from a team October 6, 2021 15:41

johnjenkins approved these changes Oct 6, 2021

View reviewed changes

splitinfinities approved these changes Oct 6, 2021

View reviewed changes

rwaskiewicz merged commit 356a244 into master Oct 6, 2021

rwaskiewicz deleted the rwaskiewicz-rebase-again-sourcemaps branch October 6, 2021 16:31

rwaskiewicz mentioned this pull request Mar 27, 2023

fix(compiler): sourcemap for dist-custom-elements generation #4200

Merged

14 tasks

feat(compiler): consumer sourcemap support #3005

feat(compiler): consumer sourcemap support #3005

Conversation

rwaskiewicz commented Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

johnjenkins Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

splitinfinities left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnjenkins left a comment

Choose a reason for hiding this comment

splitinfinities left a comment

Choose a reason for hiding this comment

fohlin commented Oct 7, 2021

LukePammant commented Oct 7, 2021

theo-staizen commented Oct 12, 2021

rwaskiewicz commented Aug 17, 2021 •

edited

Loading

splitinfinities Aug 19, 2021 •

edited

Loading

rwaskiewicz Sep 21, 2021 •

edited

Loading

johnjenkins Sep 21, 2021 •

edited

Loading

rwaskiewicz Oct 6, 2021 •

edited

Loading