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(app/ts): add TS version of PWA files #8705

Closed
wants to merge 3 commits into from

Conversation

IlCallo
Copy link
Member

@IlCallo IlCallo commented Mar 29, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Documentation
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the dev branch and not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on a Electron app
  • Any necessary documentation has been added or updated in the docs (for faster update click on "Suggest an edit on GitHub" at bottom of page) or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

Copy link
Member

@yusufkandemir yusufkandemir left a comment

Choose a reason for hiding this comment

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

Nice addition 💯 Added a few suggestions for possible improvements 🚀

app/templates/pwa/ts/custom-service-worker.ts Outdated Show resolved Hide resolved
app/templates/pwa/ts/custom-service-worker.ts Show resolved Hide resolved
@IlCallo
Copy link
Member Author

IlCallo commented Apr 12, 2021

@yusufkandemir can you take another look at this and check if this works better than the previous version?

@webnoob @TobyMosque here I had to define a local tsconfig as PWA files run in a different context (the webworker one) than the DOM one.
Do you think a similar solution is needed for SSR and BEX specific files too?

@yusufkandemir
Copy link
Member

@IlCallo it doesn't work. I've added some comments about it on the discussion related to the "TS reference directive including web worker lib" solution.

@IlCallo
Copy link
Member Author

IlCallo commented Apr 12, 2021

@yusufkandemir try again copying in the ESLint config override I put there.
ESLint process was using the wrong tsconfig configuration, thus going crazy

Now VSCode intellisense should work, still working on how to tell fork-ts-checker to get the correct one

@IlCallo
Copy link
Member Author

IlCallo commented Apr 12, 2021

Fork-ts-checker automatically takes the root level ESLint file and tsconfig, this breaks the compilation.
I think it's possible to override the fork-ts-checker typescript config file reference into webpack chain only for PWA files, but I don't have any more time this week.

Will try again next weeks

@IlCallo
Copy link
Member Author

IlCallo commented Apr 13, 2021

Plot twist: it is probably ts-loader which is taking the wrong tsconfig.json.
I checked again and fork-ts-checker-webpack-plugin isn't actually setup for PWA (which is a problem, we need it for type-related checks), so the culprit should be ts-loader.

Some tweaks are surely needed to make it take the correct config, but this should be doable
@yusufkandemir do you have some time, to try this out? First making it work using the ts-loader option and then setup fork-ts-checker to enable type-related linting

This is the place where to operate:

About using triple slash references solution, the problem with that one is that it would become cumbersome to manage that folder in the moment you want to split your code in multiple folders

PS: get a clean copy of the branch from GH, as I rebased and force pushed to get latest vue3-work changes

@yusufkandemir
Copy link
Member

@IlCallo actually, it works correctly when I disable the fork-ts-checker-webpack-plugin. As those are in different webpack chains, I also would expect the custom-service-worker chain to not have the fork-ts-checker-webpack-plugin, and it doesn't have, but the plugin from the main chain checks the file in the other chain for some reason, I've even tried disabling the register-service-worker from client-entry.js to get rid of any file in src-pwa folder from the main chain's entry, but it didn't work.

Also, I've noticed that as register-service-worker.ts is in the same folder with the new tsconfig.json file now, ESLint is throwing some errors like window is not found, or doesn't identify the entries from src/env.d.ts, this looks like somewhat complicated and it would be nice if we can completely isolate the folder from the outside while having some connections like identifying env variables and such correctly if possible.

@IlCallo
Copy link
Member Author

IlCallo commented Apr 13, 2021

Does register-service-worker.ts run into the main thread? I thought it executes into service worker scope too
fork-ts-checker-webpack-plugin poisoning seems pretty strange, since they are different chains
Will see what I can get out of it, surely this isn't an easy TS problem

@rstoenescu rstoenescu closed this Jun 21, 2021
@rstoenescu rstoenescu deleted the branch quasarframework:vue3-work June 21, 2021 23:46
@rstoenescu
Copy link
Member

Hi,

This was closed by an accident. Seems like deleting vue3-work branch also closed all PRs associated with it.
Can you recreate this for "dev" branch, pls?

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.

3 participants