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

fix(json): cannot be imported by rollup #81

Merged
merged 1 commit into from
Dec 9, 2019
Merged

fix(json): cannot be imported by rollup #81

merged 1 commit into from
Dec 9, 2019

Conversation

foray1010
Copy link
Contributor

@foray1010 foray1010 commented Dec 6, 2019 •
edited
Loading

Rollup Plugin Name: json

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

it's current jsnext:main field is pointing to a wrong path, which makes rollup failed to import the plugin

NotWoods
NotWoods approved these changes Dec 6, 2019
View reviewed changes
Copy link
Member

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

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

Thank you! And congrats on your first Rollup PR :)

shellscape
shellscape requested changes Dec 6, 2019
View reviewed changes
Copy link
Collaborator

@shellscape shellscape left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@foray1010 this is not a proper pull request. You've ignored several fields of the template. Please complete the template or we'll have to close this PR.

You've marked this as a bugfix but haven't described the bug, nor how the changes fix it, nor have you included tests, which disregards this message: bugfixes and features will not be merged without tests

@foray1010
Copy link
Contributor Author

@shellscape the jsnext:main file of package.json in @rollup/plugin-json is pointing to a file that is not exist. This PR is meant to point it to correct path, which should be the same as module

I am not sure if I really need to test the changes, since it is not code related, just some misconfig

@shellscape
Copy link
Collaborator

@foray1010 that would be an other PR then, as that's metadata. Please make sure you fill out the entire template next time. The information in your reply should have been in the original post for the PR.

@foray1010
Copy link
Contributor Author

difficult to say as user cannot use this package if the path is wrong, which is definitely a bug to me

@shellscape shellscape self-requested a review December 6, 2019 17:21
@shellscape
Copy link
Collaborator

After @TrySound mentioned that jsnext:main is deprecated, I did some digging and asking around. Rollup was really the only bundler that embraced it fully. As mentioned by Rich Harris here, jsnext:main has been superseded by module, and there are numerous sources that consider it deprecated as a result (for example).

Basically it never made it out of discussion, and Rollup supported it out of necessity at the time it was implemented.

So all that goes to say is that we have sufficient evidence to remove support from @rollup/node-resolve and to remove that field from any plugins in the repo. As such, I'd like to ask that your PR remove the jsnext:main property instead of updating the value. We'll then release it as a major.

@foray1010 foray1010 mentioned this pull request Dec 9, 2019
Merged
9 tasks
@foray1010
fix(json): cannot be imported by rollup
621a1d9 
BREAKING CHANGE: removed `jsnext:main` field
@foray1010 foray1010 changed the title fix: @rollup/plugin-json jsnext:main path fix(json): cannot be imported by rollup Dec 9, 2019
@foray1010
Copy link
Contributor Author

@shellscape I have created #85 which remove all jsnext:main field in this repo
also updated this PR

shellscape
shellscape approved these changes Dec 9, 2019
View reviewed changes
Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Thanks for working with us on this one.

@shellscape shellscape merged commit 445f8ca into rollup:master Dec 9, 2019
@bidoubiwa bidoubiwa mentioned this pull request Dec 7, 2020
Merged
bors bot added a commit to meilisearch/meilisearch-js that referenced this pull request Dec 7, 2020
@bors @dependabot-preview @bidoubiwa
Merge #711 #713
04e0a1d 
711: build(deps-dev): bump @rollup/plugin-commonjs from 16.0.0 to 17.0.0 r=curquiza a=dependabot-preview[bot]

Bumps [@rollup/plugin-commonjs](https://github.com/rollup/plugins) from 16.0.0 to 17.0.0.
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/rollup/plugins/commit/f81735340819eb51b4fe62235778bad7c2cc6e02"><code>f817353</code></a> chore(release): commonjs v17.0.0</li>
<li><a href="https://github.com/rollup/plugins/commit/cd3649231f6be269c62be58c8090866619697ad2"><code>cd36492</code></a> chore(release): typescript v8.0.0</li>
<li><a href="https://github.com/rollup/plugins/commit/befff131ac679d1736d38e47201349558e640e0a"><code>befff13</code></a> chore(release): typescript v7.0.0</li>
<li><a href="https://github.com/rollup/plugins/commit/497c16f29a07fad31ac7f879e0915e0d4372f2ad"><code>497c16f</code></a> chore(release): node-resolve v11.0.0</li>
<li><a href="https://github.com/rollup/plugins/commit/c30b2c0f0560afad3b817ad1c77af17c61c2964b"><code>c30b2c0</code></a> fix(node-resolve): refactor handling builtins, do not log warning if no local...</li>
<li><a href="https://github.com/rollup/plugins/commit/23b0bf7d76d739e20e550d0cbb0a432bc3429e20"><code>23b0bf7</code></a> refactor(node-resolve)!: simplify builtins and remove <code>customResolveOptions</code> ...</li>
<li><a href="https://github.com/rollup/plugins/commit/094b105f7efcd6862db7114e3d55bed5fb41f017"><code>094b105</code></a> fix(typescript): add missing imports (<a href="https://github-redirect.dependabot.com/rollup/plugins/issues/633">#633</a>)</li>
<li><a href="https://github.com/rollup/plugins/commit/ba1fcf9d93b930853ee0e1ad6a62b2eb11db4e1f"><code>ba1fcf9</code></a> docs(node-resolve): fix import statements in examples in README.md (<a href="https://github-redirect.dependabot.com/rollup/plugins/issues/646">#646</a>)</li>
<li><a href="https://github.com/rollup/plugins/commit/21c51e0ea4755203baa8385c6a68cf7b1178319a"><code>21c51e0</code></a> feat(commonjs)!: reconstruct real es module from __esModule marker (<a href="https://github-redirect.dependabot.com/rollup/plugins/issues/537">#537</a>)</li>
<li><a href="https://github.com/rollup/plugins/commit/40eee93e6b30f41d1cd334c53b2b6c456875e57b"><code>40eee93</code></a> feat(node-resolve)!: Mark built-ins as external (<a href="https://github-redirect.dependabot.com/rollup/plugins/issues/627">#627</a>)</li>
<li>Additional commits viewable in <a href="https://github.com/rollup/plugins/compare/commonjs-v16.0.0...commonjs-v17.0.0">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=@rollup/plugin-commonjs&package-manager=npm_and_yarn&previous-version=16.0.0&new-version=17.0.0)](https://dependabot.com/compatibility-score/?dependency-name=@rollup/plugin-commonjs&package-manager=npm_and_yarn&previous-version=16.0.0&new-version=17.0.0)

You can trigger a rebase of this PR by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)



</details>

713: Remove jsnext main r=curquiza a=bidoubiwa


`jsnext:main` is deprecated: rollup/plugins#81 (comment)
Only rollup used to use it but rollup just decided to remove it as well: rollup/plugins#85

Should be merged after: #709

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Charlotte Vermandel <charlottevermandel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NotWoods NotWoods NotWoods approved these changes

@shellscape shellscape shellscape approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@foray1010 @shellscape @NotWoods