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

feat(compiler): source maps #2892

Closed
wants to merge 17 commits into from

Conversation

johnjenkins
Copy link
Contributor

@johnjenkins johnjenkins commented Apr 23, 2021

Hello!
This PR seeks to add source-maps to most stencil output - a feature that is long requested here, here, here, here, here and proposed here

The changes simply seek to join the source-map dots (and therefore track the changes back to the original source) from the first typescript transforms and transpilation, through the rollup bundling and finally into and out of the optimisation.

I've addressed and tested most output targets: dist-lazy (cjs / esm / es5 varieties), dist-collection, dist-custom-elements and dist-custom-elements-bundle.

To keep things simple, there are no soureMap options - just a boolean.
Now setting sourceMap: true within stencil.config.ts will override typescript configuration options (sourceMap and inlineSources) and generate source maps (external .js.map files with inlined sources) for all above outputs mentioned.

Let me know if I've missed something or can improve anything.

Many thanks!

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

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • They've been added here
  • Build (npm run build) was run locally

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

There are no source maps

What is the new behavior?

As described above.

Does this introduce a breaking change?

  • Yes
  • No

Manual Testing

  • Ran npm build and npm link from the root of the repo directory (on this branch)
  • Spun up a boilerplate Stencil app using the Stencil CLI npm init stencil
  • Linked my project with the instructions found in the Contributing guidelines
  • Add a sourceMap: true option to stencil.config
  • Add a console.log('hello') to my-component.ts
npm run start
  • Open browser console and click the logged message. The source opened looks the same as the original source created. 👍
npm run build
  • Source map files are added to all outputs. 👍

John Jenkins added 3 commits April 22, 2021 23:49
…t-custom-elements, dist, esm, esm-es5 and cjs. Added option to StencilConfig type options.
Added type for merge-source-map node module.
Made tsConfig get sourceMap settings from Stencil config.
@johnjenkins johnjenkins changed the title Feature - source maps feat(compiler) - source maps Apr 23, 2021
@johnjenkins johnjenkins changed the title feat(compiler) - source maps feat(compiler): source maps Apr 23, 2021
@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jun 3, 2021

To anyone interested, I built out a temporary package of all my recent PRs (and may add some other user's PRs in future too) - just until Stencil becomes more active.

This includes:
new @Prop getters / setters (#2899)
new @Mixin decorator (#2921)
new sourceMap option for all output (including integration with @Mixins) (#2892)

You can test it out with little disruption by just swapping out any reference to @stencil/core in your package.json
e.g.

"@stencil/core": "2.6.0"

becomes

"@stencil/core": "npm:@johnjenkins/stencil-community^2.4.0"

I will keep this package in-sync with @stencil/core master up until the relevant PRs get accepted or rejected.
Many thanks!

@sandergroenensimbuka
Copy link

sandergroenensimbuka commented Jun 8, 2021

thnx @johnjenkins for the code and the package. For anyone interested, using this branch i was able to get Vscode chrome debugger (https://github.com/microsoft/vscode-chrome-debug/blob/master/README.md), which needs sourcemaps, working with a stenciljs project i was working on. I had to put some time into getting everything right but its working now. For anyone interested this is my launch.json config which works with a project dir structuring where "src" contains your original code and "www" contains the build files from stencil:

{
     "version": "0.2.0",
    "configurations": [
        {
            "type": "pwa-chrome",
            "request": "launch",
            "name": "Debug chrome",
            "userDataDir": true,
            "port": 9222,
            "url": "http://localhost:3333/",
            "webRoot": "${workspaceFolder}/www",
            "sourceMaps": true,
            "sourceMapPathOverrides": {
                "./src/*": "${workspaceFolder}/src/*",
            },
            "trace": true,    
        }
        
    ]
}

@sandergroenensimbuka
Copy link

@johnjenkins i ran into an issue with the typescript optional chaining / nullish operator. I i use you stencil package i get a runtime error on this line person?.hasEMailAddress?.emailAddress with error Uncaught TypeError: Cannot read property 'emailAddress' of undefined. Which is exactly what that operator should prevent. If i lookup the built code in js it is stated as person.hasEMailAddress.emailAddress, so no ? operator present. When i change back to the original stencil core package, delete the www dir and rebuild there is no problem and the build file contains the ? operator. Any idea what this could be?

@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jun 9, 2021

hi @sandergroenensimbuka - nope, no idea, but thanks for letting me know.
I'll see if I can recreate

edit can recreate, will fix

@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jun 9, 2021

I've found the problem and it's related to the one 3rd party lib I'm using for the @Mixin decorator. I've raised an issue (wessberg/ts-clone-node#5).
I'll give it a few days to see if I get a response (the maintainer seems pretty active).

@johnjenkins
Copy link
Contributor Author

@sandergroenensimbuka Whilst waiting for an official fix, I've pushed my own for now :) (I think!)
try @johnjenkins/[email protected]

@johnjenkins johnjenkins mentioned this pull request Jun 11, 2021
@sandergroenensimbuka
Copy link

Thnx! its working now with your 0.0.3 version

@blahah
Copy link

blahah commented Jun 25, 2021

@johnjenkins would be interested to talk if you're ever looking for a job. Tbh Ionic should be trying to hire you.

@nphyatt
Copy link
Contributor

nphyatt commented Jul 1, 2021

@johnjenkins My name is Nick and I'm the director of engineering at Ionic. Been digging through some of you PRs and would be interested in chatting with you if you're up for it! You can email me at [email protected] if you're up for it!

@trollr
Copy link

trollr commented Jul 8, 2021

Give this guy a new job and merge the PR pls! It's kinda horrible to debug code without sourcemaps

@AartdenBraber
Copy link

I would love for this to work!
@johnjenkins I'm getting this error though:
[ ERROR ] Debug Failure. Invalid raw text at Bt (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:8610:726546) at Object.createTemplateMiddle (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:8610:686287) at cloneTemplateMiddle (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:55132:26) at executeCloneNode (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:56551:12) at nextNode (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:56288:17) at Object.nextNode (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:56279:96) at cloneTemplateSpan (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:55124:157) at executeCloneNode (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:56545:12) at nextNode (C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:56288:17) at C:\Users\abr30202\AppData\Roaming\npm\node_modules\@johnjenkins\stencil-community\compiler\stencil.js:56335:28

@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jul 16, 2021

@AartdenBraber thanks so much for being a guinea pig!
This looks like an issue with @Mixin < I know that's not what you're interested in, but still, if I can ask you a massive favour, can you create a minimum reproduction repo?
I never did test on windows :s

edit - seems to be a typescript issue microsoft/TypeScript#35374
My guess is you have a template literal somewhere?

@AartdenBraber
Copy link

@johnjenkins yeah I could, would that help? Also, I'm getting a TS error that I shouldn't be using 'class' in my jsx, but rather 'className'. I didn't get that before though

@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jul 16, 2021

it would really help :)
hm that's interesting re class - I'm using that build against my component lib and not seeing that.
Both class="one" and class={{ 'two': true }} type syntax?
in your tsconfig it's:

"jsx": "react",
"jsxFactory": "h"

?

@AartdenBraber
Copy link

Yes both syntaxes, and tsconfig is
"allowSyntheticDefaultImports": true, "allowUnreachableCode": false, "declaration": false, "experimentalDecorators": true, "lib": [ "dom", "es2017", "es2018", "es2019", "es2020", "ESNext" ], "moduleResolution": "node", "module": "esnext", "target": "es2017", "noUnusedLocals": true, "noUnusedParameters": true, "jsx": "react", "jsxFactory": "h"

@johnjenkins
Copy link
Contributor Author

thanks again, If you can create the repo, I'll see what's up :D

@AartdenBraber
Copy link

Could be some trouble linking as well, I'm not sure. I have invited you to my private repo 👍

@johnjenkins
Copy link
Contributor Author

@AartdenBraber you’re a true gentleman and scholar - Thank you!

@johnjenkins
Copy link
Contributor Author

I'm awaiting more info from AartdenBraber, but whilst I wait, I've pushed a new build

@johnjenkins/[email protected]

This stops components that are not using @Mixin being touched by the mixin process (as should have been the case from the start) < so this issue should disappear for those just wishing to try sourceMaps.

I'll fix the issue with template literals and @Mixin when I can recreate the issue.

@splitinfinities
Copy link
Contributor

splitinfinities commented Jul 29, 2021

Oh buddy. Here we go. Review incoming! 🥁 Thanks for the patience!

@johnjenkins johnjenkins requested a review from a team July 29, 2021 21:10
@johnjenkins
Copy link
Contributor Author

@splitinfinities thanks so much! Your PR looks all good - after some cursory checks, have merged.

@AartdenBraber
Copy link

AartdenBraber commented Jul 30, 2021

Good news: I'm not getting any errors in my big project! This means a deal, because my team and I have been exploring the limitations of Stencil for quite some time now. I'm sure we didn't cover all possibilities, but we did cover a lot.

And even better news, John's implementation works perfectly. I can't tell you how psyched I was when I set a breakpoint and it actually froze the page on execution haha. I missed that a lot. ❤

I'm hoping it doesn't take too long to merge this, but for all Windows users: I simply copied John's implementation to @stencil/core, which did the trick.

rwaskiewicz and others added 2 commits August 6, 2021 10:15
- upgrade prettier to v2.3.2
  - lock version to prevent breaking changes in minor versions
- add prettier.dry-run package.json script
- add pipeline action to evaluate format status
- add prettierignore file for faster runs

STENCIL-8: Add Prettier to Stencil
@gavindoughtie-aon
Copy link

See #3005

@johnjenkins
Copy link
Contributor Author

Closing as this is now being integrated by the stencil core team - thanks everyone!
#3005

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Source-Maps Sourcemaps for node and browser Add Source-Maps Lack of source maps
10 participants