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

Fix sourcemap paths #3059

Merged
merged 3 commits into from
Apr 12, 2019
Merged

Conversation

justingrant
Copy link
Contributor

Fixes #3058.

Description of changes:

This PR adds shared logic to all Amplify packages' webpack.config.js files which will fix the sources array of sourcemap files so that those files can be found by the VSCode debugger and other IDEs.

This is needed because packages at build time live in different (relative) folder locations than the folders where packages will be installed from npm. These folder changes mean that end users can't easily debug into Amplify code using IDEs like VSCode. For example, breakpoints will be ignored, and "step into" will debug into confusing transpiled scripts instead of original TS or ES6 code.

For example, /packages/aws-amplify/src/index.ts imports @aws-amplify/ui. At build time, the aws-amplify and amplify-ui folders are siblings, but when the aws-amplify package is installed from npm, the amplify-ui folder is installed at ./node_modules/@aws-amplify/ui, which is a new name and is no longer a sibling to ./node_modules/aws-amplify like it was at build time.

This PR changes paths inside Amplify sourcemaps to conform to VSCode's default source mapping options here: https://github.com/Microsoft/vscode-chrome-debug#sourcemaps. The following changes are made:

  1. sourcemap paths that point to node_modules dependencies (e.g. lodash) are mapped to webpack:///./node_modules/*
  2. sourcemap paths that point to sibling packages under the @aws-amplify alias (like the UI example above) are mapped to webpack:///./node_modules/@aws-amplify/*
  3. other paths, e.g. relative paths in the same package, or webpack or node builtins, will be left alone (same behavior as current webpack config).

IMPORTANT: if new packages are added to Amplify, they should be added the folder-to-package mapping object in webpack-utils.js in this PR.

Here's an excerpt of sourcemaps generated with this PR. Note that these paths match the "desired" paths from #3058.

{  
  "version":3,
  "sources":[  
    "webpack:///webpack/universalModuleDefinition",
    "webpack:///aws-amplify.js",
    "webpack:///webpack/bootstrap 1f06f3c00a3065bca915",
    "webpack:///./node_modules/aws-sdk/lib/core.js",
    "webpack:///./node_modules/@aws-amplify/core/lib/index.js",
    "webpack:///./node_modules/graphql/module/error/index.js",
    "webpack:///./node_modules/graphql/module/type/definition.js",
    "webpack:///./node_modules/graphql/module/language/kinds.js",
    "webpack:///./node_modules/aws-sdk/lib/util.js",
    "webpack:///./node_modules/@aws-amplify/core/lib/Logger/index.js",

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

//
// IMPORTANT: if new packages are added to Amplify, add them to the map below.

const packageFolderMap = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@justingrant Thanks for the PR!
I wonder if we could get this object dynamically via @lerna/list

@alebiavati alebiavati added bug Something isn't working Build Related to build issues labels Apr 11, 2019
@justingrant
Copy link
Contributor Author

justingrant commented Apr 11, 2019

@alebiavati - The least-dependency way to automate the folder->package mapping would probably be to just enumerate the folders inside the packages directory and then pull out the name property from package.json in each folder.

Parsing the results from lerna list is another option, although I'd be inclined to go with the former approach to avoid a new dependency. (unless lerna list adds some additional value-- I'm not familiar enough to know for sure)

Another option is just to update the mapping manually. Automating is good, but if new packages are only added very rarely, then it may not be worth the extra complexity.

I'm happy to do any of these options. I don't feel strongly either way. What do you think?

Copy link
Contributor

@alebiavati alebiavati left a comment

Choose a reason for hiding this comment

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

We already use lerna so this wouldn't be an extra dependency. Lerna list returns both the NPM name and the folder name, so that could be used:
https://github.com/lerna/lerna/tree/master/commands/list#--json

Unfortunately that functionality is not exactly the same for our version of Lerna. We use v2.11.0 at the moment. We are going to upgrade as part of a different task so I'm merging this PR. We will refactor and make this map dynamic once we upgrade Lerna.

@ghost ghost assigned alebiavati Apr 12, 2019
@ghost ghost added the review label Apr 12, 2019
@alebiavati alebiavati merged commit c2ee264 into aws-amplify:master Apr 12, 2019
@ghost ghost removed the review label Apr 12, 2019
justingrant added a commit to justingrant/amplify-js that referenced this pull request Oct 7, 2020
This commit fixes a remaining handful of sourcemap issues remaining
after my previous sourcemap-related PRs aws-amplify#3059 and aws-amplify#2680:
* adds mapping for new folders: predictions, datastore,
  amplify-ui-angular, amplify-ui-components, amplify-ui-react,
  amplify-ui-storybook, amplify-ui-vue, api-graphql, api-rest
* removes webpack:/// prefix from oddball, non-file paths emitted by
  webpack (e.g. `webpack/universalModuleDefinition` or `fs (ignored)`).
  This prevents warnings from source-map-loader and related tools.
* The same code that fixes the "weird path" fix above also fixes a
  problem with aws-amplify#3059 where it would pre-pend webpack:/// as a
  fall-through case, even when the path was already a valid relative URL
  and didn't need that prefix. Also, according to @loganfsmyth (the most
  knowledgeable sourcemap expert I know), OSS library sourcemaps
  should always use relative URLs and should never use the
  webpack:/// prefix. Therefore, this PR removes that prefix.
  * updates the amplify-ui package's webpack config to emit sourcemaps
  * fixes paths to CSS files in the sourcemaps of amplify-ui package
  * adds sourcemaps for aws-amplify-vue package
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working Build Related to build issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source maps refer to invalid dependency paths
2 participants