Skip to content

feat: bundle faker with microbundle#217

Closed
prisis wants to merge 2 commits into
faker-js:mainfrom
PrisisForks:feature/bundling
Closed

feat: bundle faker with microbundle#217
prisis wants to merge 2 commits into
faker-js:mainfrom
PrisisForks:feature/bundling

Conversation

@prisis

@prisis prisis commented Jan 18, 2022

Copy link
Copy Markdown
Member

No description provided.

@netlify

netlify Bot commented Jan 18, 2022

Copy link
Copy Markdown

❌ Deploy Preview for vigilant-wescoff-04e480 failed.

🔨 Explore the source changes: 228fa50

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61eb3de4aebefc000765950f

@Shinigami92 Shinigami92 self-requested a review January 18, 2022 12:48
@Shinigami92 Shinigami92 linked an issue Jan 18, 2022 that may be closed by this pull request
@Shinigami92 Shinigami92 added the p: 3-urgent Fix and release ASAP label Jan 18, 2022
@Shinigami92

Copy link
Copy Markdown
Member

I assume this is blocked by #152 and #169 ?

@faker-js/maintainers Could we have some reviews/approvals for these other PRs? Then we can proceed with this setup 🚀

@prisis

prisis commented Jan 18, 2022

Copy link
Copy Markdown
Member Author

Yeah, after both are merged this should work :)

@JessicaSachs

Copy link
Copy Markdown
Contributor

I'd like to block on this. I don't know why we would drop support for export default and make this a breaking change.

@Shinigami92

Copy link
Copy Markdown
Member

I'd like to block on this. I don't know why we would drop support for export default and make this a breaking change.

We don't 👀

@JessicaSachs

Copy link
Copy Markdown
Contributor

Spoke with Shini and I understand what this is about now 👍🏻

Comment thread src/index.ts Outdated
Comment thread src/index.ts Outdated
Comment thread package.json Outdated
@Shinigami92

Copy link
Copy Markdown
Member

We also need to add dist and remove lib to/from some ignore files

Comment thread package.json
"unpkg": "./dist/faker.umd.js",
"files": [
"CHANGELOG.md",
"CHANGELOG_old.md",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here in files, we can remove

    "index.d.ts",
    "index.js",
    "lib",
    "locale",
Comment thread package.json
"vitepress": "^0.21.4",
"vitest": "~0.1.24"
"vitest": "~0.1.24",
"microbundle": "^0.14.2"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please move it up to sorted position
Also it would be good to use ~ instead of ^

The use rm -Rf node_modules pnpm-lock.yaml && pnpm install and commit both files

Comment thread package.json
@@ -84,7 +91,8 @@
"vinyl-transform": "^1.0.0",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should remove some dependencies:

  • browserify
  • gulp
  • gulp-gh-pages
  • gulp-rename
  • gulp-uglify
  • optimist
  • ink-docstrap
  • through2
  • vinyl-buffer
  • vinyl-source-stream
  • vinyl-transform

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can also remove the build folder completely
(and all references to that, like in readme/docs and ignore files)

@Shinigami92

Shinigami92 commented Jan 22, 2022

Copy link
Copy Markdown
Member

This PR may be superseded by #257

@prisis prisis closed this Jan 22, 2022
@prisis prisis deleted the feature/bundling branch January 23, 2022 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p: 3-urgent Fix and release ASAP

3 participants