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.

Sign up for GitHub

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

Jump to bottom

feat: bundle faker with microbundle #217

Closed
wants to merge 2 commits into from
Closed

feat: bundle faker with microbundle #217

wants to merge 2 commits into from

Conversation

prisis
Copy link
Member

@prisis prisis commented Jan 18, 2022

No description provided.

@netlify
Copy link

netlify bot commented Jan 18, 2022 •
edited
Loading

❌ 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
Setup a new bundling process #174
Closed
@Shinigami92 Shinigami92 added the p: 3-urgent Fix and release ASAP label Jan 18, 2022
@Shinigami92
Copy link
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
Copy link
Member Author

prisis commented Jan 18, 2022

Yeah, after both are merged this should work :)

@JessicaSachs
Copy link
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
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
Contributor

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

This was referenced Jan 18, 2022
Merged
Closed
@Shinigami92
Copy link
Member

@prisis try to at these lines to our workflow ci.yml

https://github.com/vitejs/vite/blob/56d84fb5ea454255ed07b9e01cf9f429eab11b33/.github/workflows/ci.yml#L3-L6

Shinigami92
Shinigami92 requested changes Jan 19, 2022
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 mentioned this pull request Jan 20, 2022
Closed
@Shinigami92 Shinigami92 linked an issue Jan 20, 2022 that may be closed by this pull request
ts ECMAScript Consider using 'export default' #245
Closed
Shinigami92
Shinigami92 requested changes Jan 21, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

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

Shinigami92
Shinigami92 requested changes Jan 21, 2022
View reviewed changes
package.json
},
"main": "./dist/faker.cjs",
"module": "./dist/faker.module.js",
"unpkg": "./dist/faker.umd.js",
"files": [
"CHANGELOG.md",
"CHANGELOG_old.md",
Copy link
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",

package.json
@@ -84,7 +91,8 @@
"vinyl-transform": "^1.0.0",
"vite": "~2.7.13",
"vitepress": "^0.21.4",
"vitest": "~0.1.24"
"vitest": "~0.1.24",
"microbundle": "^0.14.2"
Copy link
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

Shinigami92
Shinigami92 requested changes Jan 21, 2022
View reviewed changes
package.json
@@ -84,7 +91,8 @@
"vinyl-transform": "^1.0.0",
Copy link
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
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
Copy link
Member

Shinigami92 commented Jan 22, 2022 •
edited
Loading

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
Reviewers

@Shinigami92 Shinigami92 Shinigami92 requested changes

Assignees

@prisis prisis

Labels
p: 3-urgent Fix and release ASAP
Projects
None yet
Milestone
v6.0 - Ensure Project Foundation
Development

Successfully merging this pull request may close these issues.

ts ECMAScript Consider using 'export default' Setup a new bundling process
3 participants
@prisis @Shinigami92 @JessicaSachs