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

chore: rename import @tutorialkit/test-utils to test-utils #273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Nemikolh
Copy link
Member

This PR reverts the renaming of the import back to how it was: test-utils (was renamed to @tutorialkit/test-utils in #141).

This change aligns better with conventions we have in other repositories and also makes it easier to find what the import correspond to given the name of the package is test-utils and not @tutorialkit/test-utils. Note that we could instead change that package name to be @tutorialkit/test-utils but this is not a desirable change we want given it's not meant to be published.

Copy link

stackblitz bot commented Aug 24, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@AriPerkkio
Copy link
Member

AriPerkkio commented Aug 24, 2024

Internal packages should always use scoped package names. This is a security feature.

But if that convention hasn't been followed elsewhere, let's not follow it here either.

@Nemikolh
Copy link
Member Author

@AriPerkkio How is it a security feature?

@AriPerkkio
Copy link
Member

- import { resetProcessFactory, setProcessFactory } from '@tutorialkit/test-utils';
+ import { resetProcessFactory, setProcessFactory } from 'test-utils';

This could import test-utils npm package, not our own internal one. In larger organizations it's quite common that all internal packages are required to be scoped ones, so that it's easy to detect which ones can be trusted. This post might describe the idea better: https://github.blog/security/supply-chain-security/avoiding-npm-substitution-attacks/#tldr

But well, if this kind of convention isn't followed in other packages, it's fine to not follow it here either.

@AriPerkkio
Copy link
Member

I recommend to read this one as well if you haven't seen it earlier: https://medium.com/@alex.birsan/dependency-confusion-4a5d60fec610
It's a bit longer post but really good. 💯

@Nemikolh
Copy link
Member Author

Oh thanks for the pointers! Maybe we could use an import path that cannot be published then like starting with ~ or something. Let's discuss it with everyone else on Monday then.

I don't like using the @tutorialkit prefix because it's confusing as where this is coming from and I don't want the package to be accidentally published.

@Nemikolh
Copy link
Member Author

@AriPerkkio I finished reading both, thanks again for sharing! 🤩

The behaviour of --extra-index-url in python is wild 😱

@AriPerkkio
Copy link
Member

like starting with ~ or something.

That would work. I've seen ~, @/ and # prefixes used a lot for this. The # seems to be related to sub path imports but I haven't used that before. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants