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

Rewrite in TypeScript (WIP) #10

Merged
merged 26 commits into from
Sep 3, 2021

Conversation

sduduzog
Copy link
Collaborator

@sduduzog sduduzog commented Aug 5, 2021

What kind of change does this PR introduce?

  • Switch from JavaScript to TypeScript
  • Add install step and build step to github publish action
  • Expose supabase types as exports so that the user does not have to install '@supabase/supabase-js' for types

What is the current behavior?

What is the new behavior?

  • Plugin writen in typescript
  • Changed folder structure, guided by tsconfig.json
  • Generates bundles for commonjs, es modules, and typings from a single source
  • Exposes supabase types and interfaces as exports
    users can now do:
import { AuthUser, SupabaseClient } from 'vue-supabase';
...
  • The project should prebuild automatically when published to npm, for using from github, the project should build during install

Additional context

Previously, the plugin worked without any build step and with the change in file structure along with the package.json, it's important to test first that the npm release works as intended, that correct files are exposed and that they are resolved correctly.

It was previously easy to pretest PRs by installing a package from a github branch directly.

Given that the above tests pass, this PR will be ready but also, it's worth investigating if it's possible to build files on "postinstall" hook by npm

EDIT
Turns out the build step is not required.
There's an npm hook that is called before the package is published and also called after the user runs npm install or yarn install
This is where we build our typescript to create multiple bundles.

src/index.ts Outdated Show resolved Hide resolved
@sduduzog
Copy link
Collaborator Author

sduduzog commented Aug 6, 2021

@scottrobertson should I go ahead and setup eslint and/or prettier?

EDIT: Never mind. That would be completely out of scope for this PR

@scottrobertson
Copy link
Contributor

@sduduzog will this introduce any breaking changes at all with Vue2, Vue3 or Nuxt/Vue2?

@sduduzog
Copy link
Collaborator Author

sduduzog commented Aug 6, 2021

@sduduzog will this introduce any breaking changes at all with Vue2, Vue3 or Nuxt/Vue2?

It should not. In fact, it should be exactly as it was with valilla javascript.

I do however hope to give this PR more love. I still want to check if there are parts of it that could be done better, i.e. a production example library that took similar or alternative routes that I could learn from.

After that, I will need your help with testing a release, something like an alpha or beta idk. Just to make sure that the package.json changes to file routing after publishing still resolve correctly.

@scottrobertson
Copy link
Contributor

Awesome :) let me know if there is anything I can do.

@sduduzog
Copy link
Collaborator Author

sduduzog commented Aug 7, 2021

Interesting note:

By providing types, vscode infers them even for a javascript project with autocomplete for plugin options
image

@sduduzog
Copy link
Collaborator Author

sduduzog commented Aug 7, 2021

@scottrobertson It turns out, the workflow build step might not be required at all, with the prepare script added to package.json, the code is built automatically. This also means I have to rename this PR to reflect what is actually going on

@sduduzog sduduzog changed the title Introduce build step (WIP) Rewtite in TypeScript (WIP) Aug 7, 2021
@sduduzog sduduzog changed the title Rewtite in TypeScript (WIP) Rewrite in TypeScript (WIP) Aug 7, 2021
@sduduzog
Copy link
Collaborator Author

Here's where I need help @scottrobertson

Could you please create a release just so that we test out the package is being packaged properly.
With the changes here's what we should expect:

  • When npm publish is called, the script prepare inside package.json is called before the package is packed thus creating our bundles,
  • When a user runs npm install inside their project, the same thing happens, the bundles are produced.

What I noticed with testing though is that when using yarn with vite, not all the bundles are created. The types file is not outputed and the esm file is also not outputed which shouldn't be the case. This is why we need a test release, to see if these files are packed when a release is published. There's a high chance that this was an anomaly with my own project but rather be safe than sorry later.

@scottrobertson
Copy link
Contributor

Can do :) away today but I will sort it tomorrow morning.

@sduduzog
Copy link
Collaborator Author

Also, I think you should do the npm release manually..., as to not create a tag on github as when a workflow is in use... I"m not sure too with this one but seems cleaner

@scottrobertson
Copy link
Contributor

@sduduzog

❯ npm publish

> [email protected] prepare .
> run-s build


> [email protected] build /Users/scottrobertson/Code/gh/scottrobertson/vue-supabase
> run-p build:*


> [email protected] build:cjs /Users/scottrobertson/Code/gh/scottrobertson/vue-supabase
> tsc -p config/tsconfig.cjs.json


> [email protected] build:esm /Users/scottrobertson/Code/gh/scottrobertson/vue-supabase
> tsc -p config/tsconfig.esm.json


> [email protected] build:types /Users/scottrobertson/Code/gh/scottrobertson/vue-supabase
> tsc -p config/tsconfig.types.json

npm notice
npm notice 📦  [email protected]
npm notice === Tarball Contents ===
npm notice 1.3kB dist/cjs/index.js
npm notice 1.1kB dist/esm/index.js
npm notice 1.2kB package.json
npm notice 113B  config/tsconfig.cjs.json
npm notice 111B  config/tsconfig.esm.json
npm notice 371B  tsconfig.json
npm notice 148B  config/tsconfig.types.json
npm notice 795B  dist/cjs/index.js.map
npm notice 869B  dist/esm/index.js.map
npm notice 1.0kB README.md
npm notice 815B  dist/types/index.d.ts
npm notice 1.3kB src/index.ts
npm notice 369B  .github/workflows/npm-publish.yml
npm notice 227B  .github/release-drafter.yml
npm notice 304B  .github/workflows/release-drafter.yml
npm notice === Tarball Details ===
npm notice name:          vue-supabase
npm notice version:       2.1.0-beta1
npm notice package size:  3.3 kB
npm notice unpacked size: 10.2 kB
npm notice shasum:        78157c17a02b4201ea1910df78682d8aa04a0929
npm notice integrity:     sha512-d3lH+1YeHb5Vm[...]LdEYArf2roPlw==
npm notice total files:   15
npm notice
+ [email protected]

@sduduzog
Copy link
Collaborator Author

@scottrobertson Thank you. I'll test it out but I'm not happy with yarn complaining when I'm installing this from github. I fear it might be some sneaky bug hiding somewhere within the setup or I'm doing something wrong. I still need more information before I'm comfortable with this PR

@sduduzog
Copy link
Collaborator Author

Actually it fails even installed from npm...

I'll work on that now. @scottrobertson

@sduduzog
Copy link
Collaborator Author

There's a high chance that my issue was caused by the fact that I didn't have a .npmignore so the built files were being deleted by npm because they are flagged inside .gitignore 😭

One more favour, Could you please create another beta release when you can @scottrobertson

@scottrobertson
Copy link
Contributor

2.1.0-beta2 tagged

@sduduzog
Copy link
Collaborator Author

sduduzog commented Aug 14, 2021

Thanks for these releases, I'll remove the WIP when I've tested across multiple project types:

  • Vite + Vue 3.x + TS
    • Dev server
    • Build
    • Minor typescript issue discovered, a commit to fix this has been pushed to this PR,
  • Vue cli + Vue 2.x + TS
    • Dev server
    • Build
  • Nuxt + TS
    • Dev server
    • Build

@sduduzog
Copy link
Collaborator Author

sduduzog commented Sep 3, 2021

@scottrobertson I finally got time to test this with my projects. We're all good to go. Anything coming our way I'll be on hand to tackle

@scottrobertson scottrobertson merged commit 9aaaade into supabase-community:main Sep 3, 2021
@scottrobertson
Copy link
Contributor

Thank you @sduduzog

Want me to trigger a release?

@sduduzog
Copy link
Collaborator Author

sduduzog commented Sep 3, 2021

Yes

@sduduzog sduduzog deleted the introduce_build_step branch November 1, 2021 11:42
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.

2 participants