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

Docs: Update main.js format in docs/tutorials/recipes #20801

Merged
merged 1 commit into from
Jan 31, 2023
Merged

Docs: Update main.js format in docs/tutorials/recipes #20801

merged 1 commit into from
Jan 31, 2023

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Jan 26, 2023

Telescoping off of #20797

What I did

This PR updates every piece of documentation that touches main.js to use the new format. Please refer to #20797 for more context.

How to test

Check whether the changes make sense or not.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@yannbf yannbf requested a review from jonniebigodes January 26, 2023 16:33
@yannbf yannbf force-pushed the docs/default-exports-in-main branch from 3cd7df1 to d2441f8 Compare January 26, 2023 17:57
jonniebigodes
jonniebigodes requested changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@yannbf, thanks for making this change and helping out with the docs. Appreciate it 🙏 ! I left a few small items related to this pull request for you to look at. Let me know so that we can get this merged.

code/addons/docs/README.md Outdated Show resolved Hide resolved
code/addons/docs/angular/README.md Outdated Show resolved Hide resolved
code/frameworks/sveltekit/README.md Show resolved Hide resolved
@yannbf yannbf force-pushed the docs/default-exports-in-main branch 2 times, most recently from 0e05b29 to 9df25df Compare January 27, 2023 08:27
@yannbf yannbf mentioned this pull request Jan 27, 2023
Merged
5 tasks
@IanVS
Copy link
Member

IanVS commented Jan 27, 2023

I wonder, is it time to start naming the file main.mjs now, to be explicit that this is an ES module? Then it doesn't matter what the project's "type" is set to in their package.json.

IanVS
IanVS reviewed Jan 27, 2023
View reviewed changes
Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

I found a few require() statements hanging around.

code/frameworks/ember/README.md Outdated Show resolved Hide resolved
docs/snippets/common/storybook-main-config-remark-options.js.mdx Outdated Show resolved Hide resolved
docs/snippets/common/storybook-main-csf-indexer.js.mdx Outdated Show resolved Hide resolved
docs/snippets/common/storybook-main-ts-module-resolution.js.mdx Outdated Show resolved Hide resolved
docs/snippets/common/storybook-main-using-existing-config.js.mdx Outdated Show resolved Hide resolved
docs/snippets/common/storybook-test-runner-a11y-disable.js.mdx Outdated Show resolved Hide resolved
docs/snippets/common/storybook-test-runner-helper-function.js.mdx Outdated Show resolved Hide resolved
docs/snippets/common/storybook-vite-builder-aliasing.js.mdx Outdated Show resolved Hide resolved
docs/snippets/common/storybook-vite-builder-config-env.js.mdx Outdated Show resolved Hide resolved
docs/snippets/common/storybook-vite-builder-svelte-plugin.js.mdx Outdated
@@ -3,7 +3,7 @@

const preprocess = require('svelte-preprocess');
Copy link
Member

Choose a reason for hiding this comment

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

require

Copy link
Member

Choose a reason for hiding this comment

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

But also, this snippet doesn't seem right... But it is, I checked the way the svelte-webpack framework works.
It means it won't be possible to supply builder options to this framework, because they're all sent to the svelte-loader. @JReinhold I think we need to change this quickly, especially since it'll be a breaking change.

@yannbf yannbf force-pushed the docs/default-exports-in-main branch 2 times, most recently from d4b88d8 to f157699 Compare January 30, 2023 08:35
@yannbf yannbf force-pushed the feat/default-exports-in-main branch from b6d7fcc to 07bb9b9 Compare January 30, 2023 08:35
@yannbf yannbf force-pushed the docs/default-exports-in-main branch from f157699 to c23294a Compare January 30, 2023 14:35
Base automatically changed from feat/default-exports-in-main to next January 31, 2023 07:48
@yannbf yannbf requested a review from jonniebigodes January 31, 2023 09:04
jonniebigodes
jonniebigodes requested changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@yannbf , left one small item that we should also consider regarding NextJs that I would like your input on, as you're already doing this fantastic body of work in updating the documentation. Let me know, and we'll go from there.

Once again, thanks for the assist on this, I appreciate it 🙏

docs/faq.md
Comment on lines +105 to 108
export default {
webpackFinal: async (baseConfig) => {
const nextConfig = require('/path/to/next.config.js');

Copy link
Contributor

Choose a reason for hiding this comment

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

@yannbf I was wondering if we shouldn't adjust this here and in the next.js framework readme (i.e., here) to match?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds great, but definitely something for a separate issue. Would you mind creating an issue and tagging it with "nextjs"? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, sounds good to me! I'll create the issue in a bit.

@jonniebigodes
Copy link
Contributor

@yannbf, merge when able. I'll raise the issue with the NextJS folks and update the documentation in a separate pull request.

@jonniebigodes jonniebigodes mentioned this pull request Jan 31, 2023
Open
@yannbf yannbf merged commit 80d9808 into next Jan 31, 2023
@yannbf yannbf deleted the docs/default-exports-in-main branch January 31, 2023 12:18
@jonniebigodes jonniebigodes mentioned this pull request Feb 3, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IanVS IanVS IanVS left review comments

@jonniebigodes jonniebigodes jonniebigodes approved these changes

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yannbf @IanVS @jonniebigodes @valentinpalkovic