Skip to content

Conversation

lindsaylevine
Copy link

Fixes #43

wasn't quite sure how to replace the tests now that the manual copying is no longer a part of the plugin. making a note to add a specific test for this in next-on-netlify.

@lindsaylevine lindsaylevine added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Nov 18, 2020
@lindsaylevine lindsaylevine force-pushed the ll/use-configurable-dirs branch from 5a23158 to 981d538 Compare November 18, 2020 02:00
@lindsaylevine
Copy link
Author

lindsaylevine commented Nov 18, 2020

there's a "blocking" issue with this currently:

  1. when you don't set a publish dir in your toml, it defaults to the root of your project, and NoN empties the directory of publishDir and functionsDir when it runs - aka it wipes the entire project and you just end up with your functions_dir lol

possible solutions:

  1. force people to define a publish directory
  2. don't empty the publish directory in NoN if the publishPath !== 'out_publish'
  3. ???

package.json Outdated
"homepage": "https://github.com/netlify/netlify-plugin-nextjs#readme",
"dependencies": {
"chalk": "^4.1.0",
"cpx": "^1.5.0",
Copy link

Choose a reason for hiding this comment

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

We can move cpx to devDependencies

expect(await pathExists(PUBLISH_DIR)).toBeTruthy()
const nextOnNetlifyOptions = nextOnNetlify.mock.calls[0][0]
expect(nextOnNetlifyOptions.functionsDir).toEqual(FUNCTIONS_SRC)
expect(nextOnNetlifyOptions.publishDir).toEqual(PUBLISH_DIR)
Copy link

Choose a reason for hiding this comment

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

Sounds good 👍

Should we call the "real" next-on-netlify instead of mocking it? This would give a stronger confidence that the files are actually being copied to those directories. Generally speaking, this would be a stronger indicator that the plugin works as intended. What do you think? (Note: this would probably be better done in a separate PR)

Copy link
Author

Choose a reason for hiding this comment

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

yes, definitely. when i tried to call the "real" nextOnNetlify at first, i got errors due to the emptyDirSync stuff in the prev NoN version. now we can address this in a separate PR. here's an issue #53 :)

Copy link

Choose a reason for hiding this comment

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

Thanks for opening an issue for it! ❤️

Copy link

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Your solution 2. sounds good 👍 (as fixed in netlify/next-on-netlify#94)

I think this applies to the Functions directory too. If the user already has some Functions in their site, we don't want to remove them :)

@lindsaylevine lindsaylevine force-pushed the ll/use-configurable-dirs branch from 981d538 to c71248d Compare November 18, 2020 18:21
@lindsaylevine lindsaylevine merged commit 45e3477 into main Nov 18, 2020
@lindsaylevine lindsaylevine deleted the ll/use-configurable-dirs branch November 18, 2020 18:23
serhalp pushed a commit that referenced this pull request Apr 5, 2024
* Update actions

* Run tests if labeled

* Add NEXT_TEST_JOB

* ignore scripts for faster install

* Show test results in summary and fail action if errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: feature code contributing to the implementation of a feature and/or user facing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove intermediary copy steps

2 participants