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: sourcemaps for CSS, weird paths, /predictions #4216

Closed
wants to merge 1 commit into from

Conversation

justingrant
Copy link
Contributor

@justingrant justingrant commented Oct 19, 2019

Description of changes:

This PR fixes a remaining handful of sourcemap issues remaining after my previous sourcemap-related PRs #3059 and #2680:

  • CSS files in UI library - Currently, there's no source maps emitted for the CSS files in the amplify-ui package. This PR updates the amplify-ui package's webpack config to emit sourcemaps.
  • CSS file paths - CSS files' sourcemap paths (both existing as well as the new ones added above) were invalid because they were relative to the package root, not to the /dist or /lib subdirectory where the sourcemap files were. This PR fixes these paths by prepending a . to these paths, e.g. ./src/foo.css -> ../src/foo.css
  • Vue - enables source maps for the vue package via a configureWebpack setting in its config
  • Weird webpath paths - Webpack emits some oddball paths like webpack/universalModuleDefinition or webpack/bootstrap 21c2cca6cf7a65b395b7 or fs (ignored). The logic in Fix sourcemap paths #3059 would pre-pend webpack:/// to these paths which would trigger warnings in tools like source-map-loader that consume sourcemaps. This PR removes that prefix if a path is already a weird webpack path, which prevents those warnings.
  • relative URLs - The same code that fixes the "weird path" fix above also fixes a problem with Fix sourcemap paths #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.
  • adds support for the new predictions library in this repo

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

@justingrant
Copy link
Contributor Author

I'm not sure why CircleCI builds haven't completed. Let me know if there's something I need to do to unblock them.

@amhinson amhinson changed the base branch from master to main June 18, 2020 19:02
@amhinson
Copy link
Contributor

Thanks for the work here, and I'm sorry we couldn't get to this sooner 😕

With that said, we've rewritten our UI components so that they can be shared across all supported frameworks, so these changes are no longer necessary.

If you haven’t already, please see our migration guide and updated documentation for using @aws-amplify/ui-vue in your Vue projects:

https://docs.amplify.aws/ui/auth/authenticator/q/framework/vue

If there are any features or functionality missing, please open an issue & we’ll get it resolved:

https://github.com/aws-amplify/amplify-js/issues/new/choose

@amhinson amhinson closed this Aug 20, 2020
@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 Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants