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

feat: fix incorrect node loc #288

Merged
merged 2 commits into from
Mar 17, 2021
Merged

feat: fix incorrect node loc #288

merged 2 commits into from
Mar 17, 2021

Conversation

JounQin
Copy link
Member

@JounQin JounQin commented Mar 17, 2021

this causes issue with react/no-unescaped-entities before
so now mdx/no-unescaped-entities and eslint-plugin-react dep can be removed!

fix #287

@JounQin JounQin force-pushed the fix/no-unescaped-entities branch from 451a252 to dbc1f83 Compare March 17, 2021 06:00
@JounQin JounQin self-assigned this Mar 17, 2021
@JounQin JounQin added 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change 🐛 type/bug This is a problem labels Mar 17, 2021
this causes issue with `react/no-unescaped-entities` before
so now `mdx/no-unescaped-entities` and `eslint-plugin-react` dep can be removed!

fix #287
@JounQin JounQin force-pushed the fix/no-unescaped-entities branch from dbc1f83 to 441e521 Compare March 17, 2021 06:35
@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

@mdx-js/core Can you help to enable https://codesandbox.io/docs/ci for this repo?

@wooorm
Copy link
Member

wooorm commented Mar 17, 2021

Why is CS needed?

@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

Why is CS needed?

I want to publish to codesanbox's registry for testing for every PR without publishing to npm.

@wooorm
Copy link
Member

wooorm commented Mar 17, 2021

What does CodeSandbox check that npm test or yarn test doesn’t?

@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

What does CodeSandbox check that npm test or yarn test doesn’t?

My purpose is not related to npm test or yarn test, I mean that we can provide a testing version for users to confirm it's working as expected before publishing it to true npm registry for eveny PR.

Something like lerna publish --canary on CI, but it won't publish to npm even. I don't want to publish PR versions to true npm.

@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

@wooorm Can we merge and release first because it fixes a bug which breaks ESLint?

@ChristianMurphy
Copy link
Member

I want to publish to codesanbox's registry for testing for every PR without publishing to npm.

CodeSandbox can point to a specific branch without an integration using https://codesandbox.io/s/github
https://codesandbox.io/s/github/mdx-js/eslint-mdx/tree/fix/no-unescaped-entities for this branch at the root level, it can also be pointed to specific folders/paths for example https://codesandbox.io/s/github/mdx-js/eslint-mdx/tree/fix/no-unescaped-entities/packages/eslint-plugin-mdx

Is there a specific feature you're looking for from the integration?

@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

Is there a specific feature you're looking for from the integration?

It seems nope. I want an installable registry instead of editing source codes.

@ChristianMurphy
Copy link
Member

It seems nope. I want an installable registry instead of editing source codes.

🤔 but codesandbox isn't an install-able registry.
People wouldn't be able to pull down the version and test it on their documents locally.

@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

It seems nope. I want an installable registry instead of editing source codes.

🤔 but codesandbox isn't an install-able registry.
People wouldn't be able to pull down the version and test it on their documents locally.

https://codesandbox.io/docs/ci

This is published to our registry, so you can immediately test the library in CodeSandbox or locally without publishing to npm or elsewhere.

Did I misunderstand?

@ChristianMurphy
Copy link
Member

This is published to our registry, so you can immediately test the library in CodeSandbox or locally without publishing to npm or elsewhere.

Hmm, that is new. Interesting.
Though it opens the question, why the the codesandbox registry, instead of NPM or GitHub packages?

@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

codesandbox seems simplest to be integrated to me.

No NPM_TOKEN or GITHUB_TOKEN required.

@ChristianMurphy
Copy link
Member

codesandbox seems simplest to be integrated to me.

Simplest for who?
It sounds like yet another registry for end users to configure.

NPM_TOKEN

True, the release team is fairly small at the moment and cautious about automation

GITHUB_TOKEN

this one could/should be easier?
It's tagline is:

Streamline your workflow
Use the same GITHUB_TOKEN for all automated package uploads and downloads through Actions.

@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

Simplest for who?

For myself. 😂

this one could/should be easier?

I don't know what permissions are required, so codesanbox is the simplest one.

@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

And if someone is going to set up an automation of npm or GitHub packages, it would also be great to have.

@ChristianMurphy
Copy link
Member

/cc @mdx-js/releasers

@JounQin JounQin merged commit c52039c into master Mar 17, 2021
@JounQin JounQin deleted the fix/no-unescaped-entities branch March 17, 2021 17:48
@JounQin
Copy link
Member Author

JounQin commented Mar 17, 2021

Merge first, need to be released @wooorm

@JounQin
Copy link
Member Author

JounQin commented Mar 18, 2021

And if someone is going to set up an automation of npm or GitHub packages, it would also be great to have.

@ChristianMurphy

Just remember that, only master source codes should be published to npm as canary version.

I still think codesandbox is the best choice for PR.

@wooorm
Copy link
Member

wooorm commented Mar 18, 2021

@JounQin Why not use install from github? https://docs.npmjs.com/cli/v7/configuring-npm/package-json#github-urls


Released

@JounQin
Copy link
Member Author

JounQin commented Mar 18, 2021

@wooorm The source codes are all written in .ts, no .js files are included into git.

@wooorm
Copy link
Member

wooorm commented Mar 18, 2021

Ah 😅 That’s another good reason for using JS (+ JSDoc based TS)!


I still think codesandbox is the best choice for PR.

Do you think this will help a lot of people in the case of eslint-mdx?
I do not see a lot of codesandbox reproductions: https://github.com/mdx-js/eslint-mdx/issues?q=is%3Aissue+is%3Aclosed.
Say you wrote a PR to fix some problem, then I’m assuming you add a test for that asserts it’s fixed.
Isn’t that enough?

@JounQin
Copy link
Member Author

JounQin commented Mar 18, 2021

Ah 😅 That’s another good reason for using JS (+ JSDoc based TS)!

Writing .js with jsdoc is a bit painful and bad experience to myself.


It is not related to codesandbox reproductions, it's just a custom npm registry to install locally.

Tests added in PR only confirm the single case while the true project could be more complex, a pre-canary version is a better choice to me.

@JounQin
Copy link
Member Author

JounQin commented Mar 18, 2021

Of course, if we merge first, and publish canary version for every commit on master, that will be fine too.

@JounQin
Copy link
Member Author

JounQin commented Mar 24, 2021

@wooorm @ChristianMurphy

If we use codesanbox CI, we can test PR version locally like yarn add https://pkg.csb.dev/babel/babel/commit/c6bcf5c9/@babel/cli.

See https://ci.codesandbox.io/status/babel/babel/pr/13046/builds/112443 for instance.

So, can we enable it for this repo? (I don't understand why it should not.)

@wooorm
Copy link
Member

wooorm commented Mar 24, 2021

Looks cool!

I don't understand why it should not

Security.

You’re also asking about CodeSandbox, but you don’t care about that specifically, you want to release faster.

  • Downside with CSB is that it adds an app to the whole organization which is only used in one repo
  • Another one is that PRs are unsafe: anyone can make a PR. It’s dangerous.
  • Downside of npm tokens is that they’re per person: if I add a token here, all of my packages can be published. If that token is leaked, that’s incredibly bad

Why not publish to the GH package registry? In GH actions, you get access to a token specifically for that repository. They have security in place so that PRs are made safe.

@JounQin
Copy link
Member Author

JounQin commented Mar 24, 2021

Downside with CSB is that it adds an app to the whole organization which is only used in one repo

Is that true? I'm just asking for installing this app on this repo. And it can always be configured later.

Another one is that PRs are unsafe: anyone can make a PR. It’s dangerous.

That's why I don't want to publish PR versions to npm nor GH registry. The user must install a specific PR version, but won't get updates automatically. We don't need to change registry config at all.

Downside of npm tokens is that they’re per person: if I add a token here, all of my packages can be published. If that token is leaked, that’s incredibly bad

No npm token is required here


Security is always important to me too.

@JounQin
Copy link
Member Author

JounQin commented Mar 24, 2021

image

@wooorm
Copy link
Member

wooorm commented Mar 24, 2021

I'm just asking for installing this app on this repo.

Ah, yes, you’re right. I can add it to one or more specific repos.

No npm token is required here

I know! But this is to reiterate my stance on npm tokens


You did say “Of course, if we merge first, and publish canary version for every commit on master, that will be fine too.” before. That’s different from what you’re saying now. And such a canary version can be done with GH.

So, do you prefer CSB to publish PRs, or GH for canary releases?

@JounQin
Copy link
Member Author

JounQin commented Mar 24, 2021

You did say “Of course, if we merge first, and publish canary version for every commit on master, that will be fine too.” before. That’s different from what you’re saying now. And such a canary version can be done with GH.

I said that because it seems you disagreed to install CSB into this repo, so I proposed to release canary versions for every commit on master. And it seems true for mdx: https://github.com/mdx-js/mdx/blob/main/.github/workflows/publish.yml#L33

So, do you prefer CSB to publish PRs, or GH for canary releases?

I'd prefer CSB always.

@JounQin
Copy link
Member Author

JounQin commented Mar 24, 2021

Besides, Package Scoped tokens is now on npm's roadmap.

@wooorm
Copy link
Member

wooorm commented Mar 24, 2021

Besides, Package Scoped tokens is now on npm's roadmap.

It’s been for half a year, and it’s not moving fast.


Added CSB. Here’s an example config .codesandbox/ci.json they suggest for monorepos:

{
  // If you have a monorepo we infer your packages from your Yarn workspaces
  // or lerna configuration by default. If you want to explicitly
  // set what to build, you can fill the 'packages' field with paths to your
  // packages
  "packages": ["packages/react", "packages/react-dom"],
  "sandboxes": ["new", "vanilla"]
}

@JounQin
Copy link
Member Author

JounQin commented Mar 24, 2021

It’s been for half a year, and it’s not moving fast.

Yes, sadly.


Thanks for adding CSB! ❤️

.codesandbox/ci.json has been added in this PR, it's just not enabled before.

https://github.com/mdx-js/eslint-mdx/pull/288/files#diff-bca34ff1efba1992c546adca0b1596996778b69817a7c391d29f40806694fdc9

@wooorm
Copy link
Member

wooorm commented Mar 24, 2021

you probably have to open a new PR

@JounQin
Copy link
Member Author

JounQin commented Mar 24, 2021

Yes, #299

And it works https://ci.codesandbox.io/status/mdx-js/eslint-mdx/pr/299/builds/112534

Damn great.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧒 semver/minor This is backwards-compatible change 🐛 type/bug This is a problem 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

warning > can be escaped with >
3 participants